Also bitte schön, der Patch ist primitiv bis geht nicht... Und wie Du all die Escapes aus Deinem sed in "meinem" modsed unterbringst will ich auch noch sehen.
Das "Dein" bezog sich auf
Deinen Aufruf ... aber vergiß es einfach. Wieso allerdings ein Patch mit immerhin 14 Hunks, der auch noch einen internen Funktionsaufruf um einen offensichtlich nur für diesen speziellen Fall notwendigen Parameter erweitern muß (alle anderen "unbekannten" Parameter werden eben an den mount-Syscall weitergereicht) damit das funktioniert, am Ende "primitiv bis geht nicht" ist (mit dann immerhin auch 14 potentiellen Konfliktstellen beim Version-Bump), frage ich mich trotzdem weiterhin ... Du hast schon viel einfachere Patches abgelehnt oder ignoriert.
Die "Abfrage" des loop-Devices über sed war auch nur eine denkbare Möglichkeit, am Ende wird aus
Code:
mount -t ext2 -o loop,offset=256 /var/tmp/filesystem.image /var/tmp/fs
in der /var/post_install dann eben
Code:
loopdev=$(losetup -f) && losetup -r -f -o 256 /var/tmp/filesystem.image && mount -t ext2 $loopdev /var/tmp/fs
und da sehe ich nicht mal im Ansatz eine Notwendigkeit für "sed" und irgendwelche Escapes - von einem Mehraufwand mal ganz zu schweigen ... nur das "losetup"-Applet muß eben existieren anstelle des modifizierten "mount"-Applets.
Ich verstehe auch nicht, was Du eigentlich mit der Diskussion bewirken willst? Siehst Du irgendeinen Fehler im Patch? Wird das originale Verhalten von mount verändert?
a) Nichts, gar nichts
b) In gewisser Weise schon
c) Ja
Wenn Du diesen Patch für notwendig erachtest, dann verwende ihn einfach weiterhin. Wenn Du dann klar ansagst, daß Du den irgendwie eleganter oder was auch immer findest, ist das meinetwegen auch noch eine Begründung.
Wenn Du aber als Grund angibst:
er13 schrieb:
Die Version mit losetup wäre nicht so kurz, weil man losetup unter der Angabe des loop-Devices hätte aufrufen müssen. Das nächste freie loop-Devices hätte man aber auch erst suchen müssen (analog AVM).
dann sehe ich darin gleich zwei nicht ganz zutreffende Punkte (um das Wort "falsch" zu vermeiden) - erstens
muß man kein Loop-Device angeben und die Ermittlung des nächsten freien Loop-Devices muß mitnichten in einer zur Vorgehensweise von AVM analogen Form erfolgen - und denen habe ich widersprochen.
Klartext: Mach was Du willst mit dem Patch ... wenn Du tatsächlich der Meinung bist, er erhöht die Lesbarkeit des Codes an dieser Stelle, dann laß ihn einfach drin.
Nur wegen der schlechten Lesbarkeit (in meinen Augen) bin ich ja überhaupt
erneut darauf gestoßen ... auch wenn ich die Sinnfrage
hier schon einmal gestellt hatte. Die Antwort darauf habe ich wahrscheinlich überlesen - hätte ich diese Reaktion vorhergesehen, dann hätte ich nicht erneut gefragt. Hättest Du da bereits geschrieben: "Habe ich halt so gemacht und wenn es jetzt schon existiert, mache ich es auch nicht wieder rückgängig, war ja schließlich
meine Arbeit.", hätte ich das bereits vorher "begriffen".
Aber "handwerklich" finde ich diesen Patch tatsächlich auch nicht optimal umgesetzt, weder unter dem Aspekt der Wartbarkeit, der Lesbarkeit oder der Fehlerfreiheit. Warum da einerseits eine zusätzliche Option in "mount_option_str" als Literal eingeführt wird, wenn das später ohnehin erneut als Literal abgefragt wird, finde ich persönlich auch nicht einleuchtend. Wenn man das einfach so schreibt (und die "mount_option_str" unverändert läßt)
Code:
for (i = 0; i < ARRAY_SIZE(mount_options); i++) {
unsigned opt_len = strlen(option_str);
if (strncasecmp(option_str, options, opt_len) == 0
&& (options[opt_len] == '\0'
/* or is it "comment=" thingy in fstab? */
IF_FEATURE_MOUNT_FSTAB(IF_DESKTOP( || option_str[opt_len-1] == '=' ))
)
) {
unsigned long fl = mount_options[i];
if (fl & BB_MS_INVERTED_VALUE)
flags &= fl;
else
flags |= fl;
goto found;
}
IF_FEATURE_MOUNT_LOOP(
if (loopOffset) {
if (strncasecmp(options, "offset=", 7) == 0)
{
*loopOffset = xatoull(options+7);
goto found;
}
}
)
option_str += opt_len + 1;
}
hat das m.E. genau dasselbe Ergebnis (-2 Hunks) und ist deutlich besser lesbar, weil man sich keinen Knoten ins Hirn machen muß, wann denn nun der Teil mit den Flags abgearbeitet wird und wann nicht.
Wobei ich ohnehin anzweifeln würde, daß die Behandlung von "offset=" tatsächlich nach "parse_mount_options" gehört.
Die Frage, warum es auch bei der 7390 gepatcht wird (wo es die Notwendigkeit gar nicht gibt), stelle ich mir trotzdem und auch den Hinweis, daß bei Deiner Implementierung auch die Verwendung so einer "offset"-Angabe in Verbindung mit einem beliebigen Block-Device ohne Fehlermeldung (allerdings auch ohne Wirkung, weil ja "set_loop" nicht aufgerufen wird) möglich ist und nicht nur bei einem loop-Device wie beim Original aus util-linux (weil ENABLE_FEATURE_MOUNT_LOOP in der BB eben noch nicht heißt, daß auch "-o loop" angegeben wurde) , erspare ich Dir nicht.
Sogar die Frage, was die Ansage
Code:
The data start is moved OFS bytes into the specified file [B]or device[/B]
heißen soll, denn das Setup für das Loop-Device findet ja nur dann statt, wenn die Bedingung:
Code:
if (ENABLE_FEATURE_MOUNT_LOOP && S_ISREG(st.st_mode))
erfüllt ist, könnte einen dann noch umtreiben. Mich würde jedenfalls interessieren, wie ein "or device" gleichzeitig die S_ISREG-Abfrage erfüllen kann.
Aber dort wäre dann m.E. auch die richtige Stelle für die Auswertung eines "offset"-Parameters, weil es da dann tatsächlich um ein loop-Device geht. Wenn man dann "filteropts" als Basis der "Untersuchung" verwendet, kann man sich sämtliche Änderungen in Bezug auf "parse_mount_options" (inkl. des dritten Parameters beim Aufruf und der Änderung von "mount_option_str") ersparen und das reduziert am Ende den gesamten Patch von 14 Hunks auf zwei (1x Usage + 1x Implementierung). Die Suche nach "offset=" in "filteropts" bräuchte nicht einmal wirklich redundanten Code (ungetestet, nur "geschrieben"):
Code:
mp->mnt_fsname = NULL; // will receive malloced loop dev name
[COLOR="#008000"]long long loopOffset = 0;
char *offsetOption;
if (filteropts && (offsetOption = strstr(filteropts, "offset=")) loopOffset = xatoull(offsetOption+7);
[/COLOR]if (set_loop(&mp->mnt_fsname, loopFile, loopOffset, /*ro:*/ (vfsflags & MS_RDONLY)) < 0) {
Da fehlt zwar jede Fehlerbehandlung für "offset=irgendwelcher_unsinn", aber die existiert bisher auch nicht in dem Patch und wenn man ganz ganz sicher gehen will, müßte man noch prüfen, daß vor "offset=" entweder nichts steht (offsetOption == filteropts) oder daß es sich um ein Komma handelt (*(offsetOption - 1) == ','). Wenn man ganz gründlich sein will, müßte man noch den "offset="-Teil aus "filteropts" entfernen nach der Auswertung ... ist aber auch nur noch max. ein Zweizeiler mit der Suche nach einem Komma und dem Kopieren ab dieser Stelle+1 nach "offsetOption", wenn es existiert oder dem Schreiben von '\0' nach "offsetOption-1", wenn nicht gleichzeitig "offsetOption == filteropts" ist - dann reicht es vermutlich schon, direkt nach *filteropts einmal '\0' zu schreiben, um die Speicher-Freigabe kümmert sich dann "singlemount" später selbst (selbst das könnte man ja noch machen, das bleiben trotzdem < 10 Zeilen gesamt - ein paar wenige mehr, wenn man "beautiful code" schreiben will).
Meine Verblüffung, daß Du mit einem zusätzlichen Patch bereits vorhandene Funktionalität aus der originalen Busybox vom Upstream noch einmal nachbildest, wird dadurch jedenfalls auch nicht gemindert. Das Argument, daß es das Mount-Kommando aus "util-linux" genauso macht, ist auch nur teilweise nachvollziehbar:
mount man page schrieb:
This type of mount knows about four options, namely loop, offset, sizelimit and encryption, that are really options to losetup(8). (These options can be used in addition to those specific to the filesystem type.)
Wenn jemand anderes ankommen und mit einem Patch-Vorschlag für die Busybox auflaufen würde, der eigentlich nicht notwendig wäre (und die
Notwendigkeit sehe ich auch nach Deinen Ausführungen immer noch nicht) oder aus 14 Hunks besteht, wenn es zwei auch machen würden - dann möchte ich irgendwie nicht wissen, was Du demjenigen dann geschrieben hättest ... aber das führt wieder zu dem alten Streit zurück und den wollte ich gar nicht erneut entfachen.
Daher verstehe ich auch nicht so ganz, warum Du das auf einmal persönlich zu nehmen scheinst. Der gravierende Unterschied zwischen
Deiner und
meiner Lösung (wenn man denn so einen Unterschied überhaupt "herausarbeiten" will - ich sehe das überhaupt nicht als
meine Lösung an) ist es nun einmal, daß letztere mit den bereits in der Busybox vorhandenen Möglichkeiten arbeitet und keinen zusätzlichen Patch braucht (der ja auch mit "gepflegt" werden muß). Da aber Du ohnehin die Arbeit damit hast, ist mir das ziemlich Banane, was Du am Ende machst - solange Du nicht schreibst, es ginge eben nicht anders oder nur mit erheblich höherem Aufwand. Dem habe ich widersprochen, sonst nichts und diesen Zweifel erhalte ich (für mich) aufrecht.
Trotzdem danke für das Gespräch.
