Erstellt vor 12 Monaten

Geschlossen vor 5 Monaten

#2930 closed task (fixed)

Bump busybox version to 1.27.x

Erstellt von: er13 Verantwortlicher:
Priorität: normal Meilenstein: freetz-next
Komponente: packages Version: devel
Stichworte: Beobachter:
Product Id: Firmware Version:

Beschreibung


Anhänge (3)

busybox-version-bump-1.27.2-v6.patch (381.0 KB) - hinzugefügt von er13 vor 12 Monaten.
partially run-time tested
busybox-version-bump-1.27.2-v7.patch (384.4 KB) - hinzugefügt von er13 vor 12 Monaten.
run-time tested, still some known problems
busybox-version-bump-1.27.2-v8.patch (385.3 KB) - hinzugefügt von er13 vor 12 Monaten.
run-time tested

Alle Anhänge herunterladen als: .zip

Änderungshistorie (60)

comment:1 Geändert vor 12 Monaten durch er13

In 14444:

busybox generate-script:

  • fix depends_on and select helpers to match exactly the symbol specified, up to now these were also matching other symbols (those having the specified one as prefix)
  • refs #2930

comment:2 Geändert vor 12 Monaten durch er13

In 14445:

parse-config script:

  • fix "kconfig source statement"-pattern, the old one could accidentally match a help line starting with "source …" (busybox 1.27.2 has such a line)
  • refs #2930

comment:3 Geändert vor 12 Monaten durch er13

In 14446:

busybox generate-script:

  • replace symbols also within if-kconfig-statement
  • refs #2930

comment:4 Geändert vor 12 Monaten durch er13

In 14449:

parse-config script:

  • improve "kconfig source statement"-pattern from r14445 - support source statements with comments
  • refs r14445, refs #2930

comment:5 Antwort: Geändert vor 12 Monaten durch er13

In 14450:

  • remove local keywords incorrectly used outside of any function - busybox-1.27.2 does not tolerate them anymore
  • refs #2930

Geändert vor 12 Monaten durch er13

partially run-time tested

comment:6 als Antwort auf: ↑ 5 Geändert vor 12 Monaten durch er13

Replying to er13:

In 14450:

  • remove local keywords incorrectly used outside of any function - busybox-1.27.2 does not tolerate them anymore
  • refs #2930
  1. this upstream commit, we should probably revert it as AVM uses this keyword incorrectly in a couple of scripts

comment:7 Geändert vor 12 Monaten durch er13

In 14451:

  • remove more local keywords incorrectly used outside of any function - busybox-1.27.2 does not tolerate them anymore
  • refs #2930

comment:8 Antwort: Geändert vor 12 Monaten durch er13

In 14452:

source statement:

Geändert vor 12 Monaten durch er13

run-time tested, still some known problems

comment:9 als Antwort auf: ↑ 8 Geändert vor 12 Monaten durch ralf

Replying to er13:

specify path to the file to be sourced, busybox-1.27.2 doesn't lookup in current dir anymore

Dies entspricht POSIX.
Es steht aber nicht, dass es ein absoluter Pfad sein muss. Wenn das aktuelle Verzeichnis passt, kann man die Skripte auch mit "./" aufrufen. Noch eine andere Möglichkeit ist, das Verzeichnis in den PATH mit aufzunehmen.

comment:10 Antwort: Geändert vor 12 Monaten durch er13

Es ist mir klar, dass es nicht zwingend ein absoluter Pfad sein muss. Ich habe mich einfach an die direkt daneben liegenden Dateien dran gehalten: login.cgi, logout.cgi,pwchange.cgi.

Viel problematischer ist an der Stelle die Einschätzung, an was sich AVM über Jahre hinweg dran gehalten hat: an POSIX oder an das, was de-facto in busybox umgesetzt gewesen ist.

  • Bei source sieht momentan so aus - an POSIX.
  • Bei local dagegen nicht.

comment:11 Geändert vor 12 Monaten durch er13

Problem:

  • some AVM scripts (e.g. prepare_fwupgrade.sh, supportdata.wlan, boxfeaturedisable, wlantimectrl, etc.) use the local keyword outside of a function, i.e. at the scope level 0.
  • vanilla-busybox-1.27.2 doesn't tolerate it anymore because of ef2386b.

Possible solutions:

  1. patch all AVM scripts manually, i.e. find all affected scripts manually and provide the corresponding patches - even though the number of scripts affected within one firmware is not that big, this alternative is considered to be not appropriate because of the number of the firmwares to be checked.
  2. patch all AVM scripts automatically by e.g. using the "check the syntax"-feature provided by busybox-ash - could be possible but requires some investigation and implementation effort. Finding the affected scripts by using some simple regex-pattern is unfortunately not that easy, AVM strips the scripts by removing leading (indentation) whitespaces, making it impossible to know if the line starting with "local " is inside or outside of a function.
  3. revert ef2386b - s. below

Used the following script for analyzing the last alternative from the list above.

Test script (revert-ef2386b-test.sh):

#!/bin/sh

f() {
  local v="local v within function"
  echo "v=${v}"
}

echo "before local assignment"

local v="local v NOT within function"
echo "v=${v}"

f

echo "v=${v}"

bash-4.x behavior

before local assignment
./revert-ef2386b-test.sh: line 10: local: can only be used in a function
v=
v=local v within function
v=

I.e. local v outside of a function is completely ignored (the value is not assigned), but the script is processed until the end.

dash (=debian /bin/sh) behavior

before local assignment
./revert-ef2386b-test.sh: 10: local: not in a function

I.e. the script is processed until local v outside of a function and aborted.

vanilla-busybox-1.27.2-ash behavior

before local assignment
./revert-ef2386b-test.sh: local: line 10: not in a function
v=
v=local v within function
v=

The same behavior as bash-4.x above.

busybox-1.27.2-ash with ef2386b reverted

before local assignment
v=local v NOT within function
v=local v within function
v=local v NOT within function

local v outside of a function is tolerated, v is assigned, the script is processed until the end.

The only test under shell/ash_test (additionally) failing after reverting ef2386b is that added in it - local2.tests. All other tests show the same behavior as before reverting ef2386b.

Conclusion: it is safe to revert it. Go for the alternative 3 for now, investigate the alternative 2 later.

Geändert vor 12 Monaten durch er13

run-time tested

comment:12 als Antwort auf: ↑ 10 Geändert vor 12 Monaten durch ralf

Replying to er13:

Viel problematischer ist an der Stelle die Einschätzung, an was sich AVM über Jahre hinweg dran gehalten hat: an POSIX oder an das, was de-facto in busybox umgesetzt gewesen ist.

  • Bei source sieht momentan so aus - an POSIX.
  • Bei local dagegen nicht.

Sie haben sich vermutlich an das gehalten, was funktioniert. Ich glaube nicht, dass die Pfade bei source wegen der POSIX Kompatibilität da stehen, eher aus Gewohnheit weil man es irgendwo so schon mal so gesehen hat.

Ich gehe auch davon aus, dass local nirgendwo tatsächlich notwendig ist, auch nicht in den Funktionen. Notwendig wäre es nämlich nur dann, wenn die gleiche Variable global und lokal verwendet wird, und das wäre sowieso unübersichtlich.

Andererseits sehe ich es auch als die einfachste Lösung, die Änderung in der Shell rückgängig zu machen. Alle Skripte von AVM zu korrigieren ist weitaus mehr Aufwand. Meinst Du mit "check the syntax"-feature die Option -n? Die Option -n erkennt dies nicht.

$ ./busybox sh test.sh
sh: local: not in a function
$ ./busybox sh -n test.sh
$

Vielleicht ändert AVM selbst irgendwann die Skripte, wenn sie mal auf die aktuelle Busybox wechseln. Vielleicht patchen sie aber auch selbst nur die Änderung wieder heraus.

Trotzdem ist es gut, wenn die Freetz Skripte sich nicht darauf verlassen.

comment:13 Geändert vor 12 Monaten durch er13

In 14455:

busybox:

  • bump version to 1.27.2 (version v8 from #2930)
  • refs #2930

comment:14 Geändert vor 12 Monaten durch f_666

Könnte jemand bitte mal den Befehl "df" überprüfen?
Bei mir ergibt sowohl "df" als auch "busybox df" nur Müll (und das auch noch unterschiedlich). Bin gerade am debuggen.

comment:15 Geändert vor 12 Monaten durch er13

@df: funktioniert bei mir ohne Auffäligkeiten - zeigt den erwarteten Output.

Wie ist Müll zu verstehen? Kodierung? Oder was anderes?

comment:16 Geändert vor 12 Monaten durch ralf

Bei mir zeigt df auch keine korrekten Werte an. Ich habe zwei Probleme gefunden, die aber wohl eher in der Library liegen als in busybox.

In busybox-1.27.2/coreutils/df.c:221 wird die Funktion statfs für eine Variable vom Typ struct statfs. So weit so gut. Aber der Aufruf landet in einer Library Funktion statfs64. Ich habe deshalb mal probeweise das auf statfs64 geändert, aber das allein hat nicht geholfen, es hat aber die Werte verändert.

In der Library ist uClibc-0.9.33.2/libc/misc/statfs/statfs64.c eine Funktion, die zuerst __libc_statfs mit einer Struktur statfs aufruft und dann die Werte in eine Stuktur statfs64 kopiert. In Zeile 45 wird das Feld f_frsize kopiert, aber nur wenn _STATFS_F_FRSIZE definiert ist. Nun wird _STATFS_F_FRSIZE in uClibc-0.9.33.2/libc/sysdeps/linux/common/bits/statfs.h definiert, aber es scheint nicht aktiv zu sein. Bei mir steht im Feld f_frsize nach dem Aufruf der Wert 80, aber der stand auch schon vor dem Aufruf da. Dies ist vermutlich das zufällige Element in der Anzeige. Wenn man f_frsize auf 0 setzt, dann übernimmt df den Wert aus dem Feld f_bsize und die Ausgabe passt.

Es scheint also alles an der Library zu liegen.

  • _STATFS_F_FRSIZE sollte definiert werden, das Feld gibt es laut seit Linux 2.6 (ich habe leider keine nähere Angabe welche 2.6).
  • Wenn _STATFS_F_FRSIZE nicht gesetzt ist, sollte f_frsize auf 0 gesetzt werden und nicht uninitialisiert bleiben.
  • Wenn statfs im Programm verwendet wird, wird statfs64 aufgerufen. Das wäre soweit nicht schlimm, soweit dann auch die Struktur statfs64 ist, aber es wäre geschickter, gleich den stat64 Systemaufruf zu verwenden, denn der jetzige Aufruf liefert nur 32-Bit Wert und könnte somit Probleme mit großen Datenträgern haben.

Zur Lösung diese Problems würde es reichen, _STATFS_F_FRSIZE zu definieren oder dafür zu sorgen, dass anderenfalls f_frsize auf 0 gesetzt wird. Alternativ könnte man in df.c f_frsize auf 0 setzten bevor statfs aufgerufen wird, aber das wäre unschön.

comment:17 Geändert vor 12 Monaten durch er13

Ok, Stichwörter statfs, df - da war erst neulich was auf der busybox mailing list - click.

p.s. habe bei mir nicht auf die Korrektheit der angezeigten Werte geachtet, sondern nur nach "Müll vs. nicht Müll" geschaut. Sinnvolle Beschreibung des Fehlrverhaltens ist also doch von Bedeutung…

comment:18 Antwort: Geändert vor 12 Monaten durch er13

Die andere Frage, warum funktioniert es auf einmal nicht. Wenn ich mir das ChangeLog von coreutils/df.c so anschaue (1.24.2 wurde 2015 veröffentlicht, somit sind nur die Änderungen ab 2016 interessant), so sehe ich keinen Commit, den verantwortlich machen würde - höchstens verdächtig wäre der e184a88, aber auch der nicht wirklich.

comment:19 Geändert vor 12 Monaten durch er13

Und könntet Ihr bitte genauer das Fehlerbild beschreiben. Für welche Dateisysteme funktioniert es nicht? Für alle oder nur für entfernte (e.g. NFS)?

Ich vermute es ist gar kein neuer Bug, er war auch mit der 1.24.2 da (die uClibc/der Kernel haben sich ja beim busybox-Bump nicht geändert).

comment:20 als Antwort auf: ↑ 18 Geändert vor 12 Monaten durch ralf

Replying to er13:

da war erst neulich was auf der busybox mailing list - click.

Von statfs zu statvfs zu wechseln hat nichts mit diesem Fehler zu tun. Es würde aber helfen, falls die dabei verwendete Library-Funktion diesen Fehler nicht hat.

Replying to er13:

höchstens verdächtig wäre der e184a88, aber auch der nicht wirklich.

Nein, das ist nicht die Ursache für den Bug, es ist ein Versuch, ihn zu umgehen, allerdings nicht vollständig. Der Kommentar beschreibt genau das Problem:

Some uclibc versions were seen to lose f_frsize
(kernel does return it, but then uclibc does not copy it)

Der Kernel übergibt einen Wert für f_frsize, die Library Funktion kopiert ihn nicht. Das Problem ist, dass sie das Feld komplett ignoriert, also auch nicht auf 0 setzt, daher sind die folgenden Zeilen keine vollständige Lösung. Ich habe jetzt keine anderen Dateisysteme als die auf der Box zur Hand, aber ich habe mit dem Debugger gesehen, was passiert.
Wenn man das in df.c lösen will und nicht in der Library, dann indem man vor dem Auftruf s.f_frsize auf 0 setzt:

+ s.f_frsize = 0;
  statfs (mount_point, &s)
  if (s.f_frsize == 0)
    s.f_frsize = s.f_bsize;

Das Feld s.f_frsize behält den Wert, den es vorher hatte. Wenn es also vorher zufällig den Wert 0 hatte (oder s.f_bsize) passt alles. Es kann sein, dass man bei der vorherigen Version nur Glück hatte dass auf dem Stack an der Stelle eine 0 war.

comment:21 Geändert vor 12 Monaten durch er13

@statfs: wir haben den folgenden uClibc-Patch. Ist er fehlerhaft? Sollte der Teil

/* No support for f_frsize so set it to the full block size.  */
buf->f_frsize = fsbuf.f_bsize;

der Stand jetzt nur bei statvfs vorhanden ist, nicht auch bei statfs64 und fstatfs64 hinzugefügt werden?

Lösungsalternativen:

  • memset auf 0 von s [dies auf jeden Fall für ältere uClibc-Versionen]
  • Umstieg auf statvfs, da der Fehler bei dieser zufälligerweise richtig korrigiert wird [greift bei älteren uClibc-Versionen nicht, da bei diesen kein Patch]
  • uClibc-Patchen [dies auf jeden Fall für uClibc-0.9.33.x unabhängig von memset]

Edit:

--- coreutils/df.c
+++ coreutils/df.c
@@ -208,6 +208,7 @@
 		mount_point = mount_entry->mnt_dir;
 		fs_type = mount_entry->mnt_type;
 
+		memset(&s, 0, sizeof(s));
 		if (statfs(mount_point, &s) != 0) {
 			bb_simple_perror_msg(mount_point);
 			goto set_error;
Zuletzt geändert vor 12 Monaten von er13 (vorher) (Diff)

comment:22 Geändert vor 12 Monaten durch er13

uClibc, statfs change log - auf den ersten Block nix, was das Problem beheben würde.

comment:23 Geändert vor 12 Monaten durch ralf

In uClibc-0.9.33.2/libc/misc/statfs/statfs64.c ist schon alles drin:

    buf->f_type = buf32.f_type;
    buf->f_bsize = buf32.f_bsize;
    buf->f_blocks = buf32.f_blocks;
    buf->f_bfree = buf32.f_bfree;
    buf->f_bavail = buf32.f_bavail;
    buf->f_files = buf32.f_files;
    buf->f_ffree = buf32.f_ffree;
    buf->f_fsid = buf32.f_fsid;
    buf->f_namelen = buf32.f_namelen;
#ifdef _STATFS_F_FRSIZE
    buf->f_frsize = buf32.f_frsize;
#endif

Das Problem ist, dass _STATFS_F_FRSIZE anscheinend nicht definiert ist, so dass die Zuweisung nicht ausgeführt wird.

In coreutils/df.c ist kein memset nötig, es reicht s.f_frsize = 0.

Ich würde es vorziehen, die Library zu korrigieren, oder haben wir Boxen, wo die Library nicht ersetzt wird? Das Problem betrifft schließlich alle Stellen, wo die Funktion aufgerufen wird und nachher das Feld verwendet wird.

comment:24 Geändert vor 12 Monaten durch er13

Wir haben den Patch nur für uClibc-0.9.33.x. Für alle anderen Versionen müssen wir eine andere Lösung finden (s. comment:21).

comment:25 Geändert vor 12 Monaten durch er13

In 14469:

busybox, df applet:

  • improve uClibc-statfs.f_frsize-related workaround (df applet already contains) by zeroing statfs-buffer before statfs-call
  • refs #2930

comment:26 Geändert vor 12 Monaten durch er13

Wir haben den Patch nur für uClibc-0.9.33.x. Für alle anderen Versionen müssen wir eine andere Lösung finden (s. comment:21).

Das Symbol _STATFS_F_FRSIZE ist in libc/sysdeps/linux/common/bits/statfs.h definiert. Jedoch hat die mips-Arch eine eigene Version davon libc/sysdeps/linux/mips/bits/statfs.h, in der explizit steht "Fragment size - unsupported" und das Symbol demzufolge nicht definiert wird, was wiederum dazu führt, dass der Code in #ifdef nicht greift.

Ich meine es gehört derselbe else-Zweig rein wie bei internal_statvfs.c:

...
#else
    /* No support for f_frsize so set it to the full block size. */
    buf->f_frsize = buf32.f_bsize;
#endif

Oder zumindest explizites auf 0 setzen.

comment:27 Geändert vor 12 Monaten durch ralf

Wie bereits oben geschrieben, wenn man das in df.c lösen will, reicht es, s.f_frsize auf 0 zu setzen.

Ob wir für andere Library Versionen einen Patch habe oder nicht spielt keine große Rolle, ich bin sicher, dass in den älteren Versionen die Funktion nicht viel anders aussieht. Notfalls kann man die gesamte Datei statfs64.c kopieren.

Die Aussage "Fragment size - unsupported" ist Unsinn. Warum sollte bei etwas so allgemeinem wie dem Dateisystem gerade bei MIPS dieses Feld nicht unterstützt werden? Und selbst wenn man das Feld nicht unterstützen will, dann kann man es wenigstens auf 0 setzen statt es uninitialisiert zu lassen.

Ich habe mal folgendes Programm auf der 7490 laufen lassen:

#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <sys/statfs.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>

void
prep (struct statfs64 *s)                               
{                                                       
  memset (s, 0xAA, sizeof (*s));                                                                                                                                                                                                  
  errno = 0;
}
void
result (struct statfs64 *s)                             
{                                                       
  printf ("%d:%s\n", errno, strerror (errno));                                                                                                                                                                                    
  for (int i = 0; i < sizeof (*s) / sizeof (uint32_t); i++) printf ("%08x, ", ((uint32_t*)s)[i]);
  printf ("\n");
}                                                       

int
main ()
{
  int ret, fd;
  struct statfs64 s;
  fd = open (".", O_RDONLY|O_DIRECTORY);

  prep (&s);
  ret = statfs (".", (struct statfs *)&s);
  result (&s);

  prep (&s);
  ret = fstatfs (fd, (struct statfs *)&s);
  result (&s);

  prep (&s);
  ret = statfs64 (".", &s);
  result (&s);

  prep (&s);
  ret = fstatfs64 (fd, &s);
  result (&s);

  prep (&s);
  ret = syscall (SYS_statfs, ".", &s);
  result (&s);

  prep (&s);
  ret = syscall (SYS_fstatfs, fd, &s);
  result (&s);

  prep (&s);
  ret = syscall (SYS_statfs64, ".", &s);
  result (&s);

  prep (&s);
  ret = syscall (SYS_fstatfs64, fd, &s);
  result (&s);
}

Das bringt bei mir folgendes Ergebnis:

0:Success
01021994, 00001000, 00001000, 000074e8, 0000351f, 000074e8, 00007256, 0000351f, 00000000, 00000000, 000000ff, 00001020, 00000000, 00000000, 00000000, 00000000, 00000000, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, 
...
01021994, 00001000, aaaaaaaa, aaaaaaaa, 00000000, 000074e8, 00000000, 0000351f, 00000000, 000074e8, 00000000, 00007256, 00000000, 0000351f, 00000000, 00000000, 000000ff, 00001020, 00000000, 00000000, 00000000, 00000000, 00000000, aaaaaaaa, 
...
01021994, 00001000, 00001000, 000074e8, 0000351f, 000074e8, 00007256, 0000351f, 00000000, 00000000, 000000ff, 00001020, 00000000, 00000000, 00000000, 00000000, 00000000, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, 
...
22:Invalid argument
aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, aaaaaaaa, 
... 

Das Ergebnis für die fstat Varianten ist genau gleich, deswegen habe ich das hier weggelassen.
Man sieht hier zum einen, dass nur f_frsize nicht gesetzt wird, das unmittelbar folgende Wort ist Alignment. Zum anderen wird der Syscall statfs64 nicht ausgeführt (Invalid argument), und die C Funktion verwendet statfs und kopiert dann die Ergebnisse. Ich habe aber nicht herausgefunden, warum der statfs64 Syscall nicht funktioniert, in der Syscall Tabelle ist der Eintrag enthalten (Nr 4255 und 4256) und die zugehörigen Funktionen sind im Kernel auch drin (sys_statfs64 und sys_fstatfs64).

Das Feld f_frsize wird zurückgegeben, der Wert ist 0x1000, jeweils der dritte Wert vom Ergebnis. Wenn Du eine ältere Box zum Hand hast, kannst Du das auch mit älteren Kernel-Versionen ausprobieren.

comment:28 Antwort: Geändert vor 12 Monaten durch er13

Noch ein wenig "Erkenntnisse teilen":

  • hab' nochmal die alte busybox-Version (1.24.2) getestet, in dieser tritt der Fehler NICHT auf
  • das fand ich komisch, denn sollte es an der uClibc liegen, hätte der Fehler genauso mit der älteren Version auftreten müssen
  • die Erklärung ist allerdings einfach - f_frsize wird erst seit 8f4faa1 verwendet

comment:29 als Antwort auf: ↑ 28 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:

f_frsize wird erst seit 8f4faa1 verwendet

Die Begründung dort erscheint plausibel. Es ist nicht deren Fehler, das uClibc das Feld nicht liefert.

Wenn nichts dagegen spricht, sollte man statvfs verwenden, das ist in POSIX definiert, im Gegensatz zu statfs.

Für Freetz schlage ich vor, in uClibc _STATFS_F_FRSIZE zu definieren.

comment:30 als Antwort auf: ↑ 29 Geändert vor 12 Monaten durch er13

Replying to ralf:

Wenn nichts dagegen spricht, sollte man statvfs verwenden, das ist in POSIX definiert, im Gegensatz zu statfs.

das entspricht ja genam dem von mir in comment:17 verlinkten Vorschlag auf der Mailingliste.

Für Freetz schlage ich vor, in uClibc _STATFS_F_FRSIZE zu definieren

Wenn wir den Fehler schon entdeckt und analysiert haben, sollten wir den, finde ich, auch upstream melden. In der Head-Version ist der noch enthalten. Und ich würde ihn schon in zwei Teile splitten: der fehlende else-Zweig (wie in comment:26 beschrieben) und die Frage, ob es richtig ist, dass _STATFS_F_FRSIZE bei mips nicht gesetzt wird.

Ansonsten nehme ich das Thema für das nächste Update der Download-Toolchain mit.

Zuletzt geändert vor 12 Monaten von er13 (vorher) (Diff)

comment:31 Antwort: Geändert vor 12 Monaten durch er13

Weitere Erkenntnisse:

  • es gab wohl in der Tat eine Architektur, die kein f_frsize-Feld in der statfs-Struktur hatte (vax, der Support für diese wurde allerdings inzwischen aufgegeben). Von daher ist/war es richtig, dass bei libc/misc/statfs/statfs64.c und bei libc/misc/statfs/fstatfs64.c der von mir angesprochene else-Zweig fehlt.
  • es ist allerdings unerklärlich, warum bei mips f_frsize zwar vorhanden ist, dies jedoch nicht entsprechend signalisiert wird (fehlendes Setzen von _STATFS_F_FRSIZE).

Damit sind die Vorschläge von Ralf genau richtig:

  • die allerbeste Lösung ist der Umstieg auf statvfs wie auf der Mailing-Liste vorgeschlagen, dies behebt auch das Problem für die älteren uClibc-Versionen - der else-Zweig existiert in diesen als der (einzige) Hauptzweig. Damit wird bei der Verwendung von statvfs das Feld f_frsize immer gesetzt. Und es braucht keines Workarounds, weder des Teils "auf 0 setzen davor" noch des Teils "danach prüfen, ob es 0 geblieben ist".
  • uClibc-0.9.33.x soll dahingehend gepatcht werden, dass _STATFS_F_FRSIZE bei mips gesetzt wird.

Ich mache die entsprechenden upstream-Meldungen.

comment:32 als Antwort auf: ↑ 31 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:

der else-Zweig existiert in diesen als der (einzige) Hauptzweig. Damit wird bei der Verwendung von statvfs das Feld f_frsize immer gesetzt.

Das ist aber auch nur "meistens" korrekt.

Die Änderung 8f4faa1 wurde ja durchgeführt, weil eben f_frsize der korrekte Wert ist (bei statvfs auf jeden Fall) und dieser Wert nicht zwangsläufig identisch ist mit f_bsize.

Der Kernel liefert den korrekten Wert, und wir sollten ihn auch nutzen. Spricht etwas dagegen, die Korrektur auch bei den älteren uClibc Versionen zu verwenden?

comment:33 Antwort: Geändert vor 12 Monaten durch er13

Dieser Kommentar Fragment size - unsupported stammt wohl aus den Kernel-Quellen: arch/mips/include/uapi/asm/statfs.h 3.10.x. Und ist sogar immer noch in den aktuellsten Quellen enthalten arch/mips/include/uapi/asm/statfs.h 4.9.y

Es gibt jedoch keine MIPS-spezifische fs/statfs.c und in dieser scheint f_frsize unterstützt zu sein, s. statfs_by_dentry, do_statfs_native, do_statfs64

comment:34 als Antwort auf: ↑ 32 Geändert vor 12 Monaten durch er13

Replying to ralf:

Die Änderung 8f4faa1 wurde ja durchgeführt, weil eben f_frsize der korrekte Wert ist (bei statvfs auf jeden Fall) und dieser Wert nicht zwangsläufig identisch ist mit f_bsize.

Stand jetzt haben wir eine uClibc Version, in der _STATFS_F_FRSIZE nicht gesetzt wird. Damit greift der else-Zweig, damit wird f_frsize gleich f_bsize gesetzt. Ob nun busybox den Wert aus f_bsize (wie bei 1.24.x) oder aus f_frsize (1.27.x) ausliest, ist am Ende wurscht. Solange wir _STATFS_F_FRSIZE nicht setzen, bleibt es f_bsize. Edit: ich bin bei der Aussage natürlich bei der statvfs-Version von der Mailing-Liste.

Der Kernel liefert den korrekten Wert, und wir sollten ihn auch nutzen. Spricht etwas dagegen, die Korrektur auch bei den älteren uClibc Versionen zu verwenden?

Nur der damit verbundene Aufwand. Ob es technische Hürden gibt, wie z.B. "bei älteren Kernel-Versionen wird der Wert vom Kernel nicht gesetzt" weiß ich nicht.

Zuletzt geändert vor 12 Monaten von er13 (vorher) (Diff)

comment:35 Geändert vor 12 Monaten durch er13

In 14474:

uClibc-0.9.33.x:

  • mark f_frsize field of statfs/statfs64 structures as supported also on mips
  • refs #2930

comment:36 als Antwort auf: ↑ 33 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:

Dieser Kommentar Fragment size - unsupported stammt wohl aus den Kernel-Quellen:

Die einzige Erklärung, die ich dafür habe ist, dass Linux versucht kompatibel zu der Unix Variante zu sein, die auf dem Prozessor gängig ist/war, und dass diese das Feld nicht unterstützt hat, und folglich auch kein Anwendungsprogramm dieses Feld auf dem ursprünglichen Unix genutzt hat.

Wenn Linux schon das Feld in der Struktur hat und den Code im Dateisystem, dieses Feld zu füllen, gibt es keine Grund, für eine CPU den Wert dann einfach nicht herauszugeben, wenn er im Kernel schon vorhanden ist.

Ich hatte gerade Gelegenheit, das Test-Programm auf einem 2.6.13 Kernel laufen zu lassen, auch da wird das Feld übergeben.

Hast Du etwas gefunden, warum die statfs64 Systemaufrufe nicht funktionieren?

comment:37 Geändert vor 12 Monaten durch er13

In 14475:

busybox, df applet:

comment:38 Geändert vor 12 Monaten durch er13

In 14476:

busybox:

  • replace O_CLOEXEC / RTA_TABLE related patches with their latest versions
  • refs #2930

comment:39 als Antwort auf: ↑ 36 ; Antwort: Geändert vor 12 Monaten durch er13

Replying to ralf:

Hast Du etwas gefunden, warum die statfs64 Systemaufrufe nicht funktionieren?

Du rufst sie falsch auf, die beiden Syscalls erwarten 3 Parameter: s. libc/misc/statfs/fstatfs64.c, libc/misc/statfs/statfs64.c

comment:40 als Antwort auf: ↑ 39 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:

Du rufst sie falsch auf, die beiden Syscalls erwarten 3 Parameter

Ok, ich hatte nicht erwartet, dass die Parameter bei statfs64 anders sind als bei statfs. Jedenfalls kann ich bestätigen, dass auch mit dem alten Kernel 2.6.13 schon der Systemaufruf statfs64 funktioniert und die gleichen Werte zurück liefert wie der Aufruf der Library Funktion, außer dass natürlich der Systemaufruf auch den korrekten Wert für f_frsize liefert. Der Vorteil, wenn man direkt den Systemaufruf verwendet ist, dass er auch Werte zurückliefern kann für große Dateisysteme. Die Funktion in der Library nutzt erst den 32-Bit Systemaufruf und konvertiert die Werte dann erst in 64 Bit. Das funktioniert natürlich nicht, wenn die Werte nicht in 32 Bit passen.

Da fragt man sich, warum nicht direkt der 64-Bit Systemaufruf verwendet wird. Der Aufbau von statfs64.c ist so:

#if defined __NR_statfs
// 64-Bit Funktion wird auf 32-Bit Aufruf abgebildet
#else
// 64-Bit Kernel-Aufruf wird direkt genutzt (__NR_statfs64)
#endif

Ich weiß nicht, was die sich dabei gedacht haben. Den Kernel Systemaufruf statfs gibt es immer. Auf x86 gibt es nur statfs und kein statfs64. Das liegt daran, dass statfs auf dem 64-Bit System schon 64-Bit Werte zurück gibt, es gibt also keinen Grund, zusätzlich noch einen weiteren Systemaufruf statfs64 einzuführen, der die gleichen 64-Bit Werte liefert.

Die Datei sollte also so geändert werden:

#if ! defined __NR_statfs64
// 64-Bit Funktion wird auf 32-Bit Aufruf abgebildet
#else
// 64-Bit Kernel-Aufruf wird direkt genutzt (__NR_statfs64)
#endif

Die 64-Bit Funktion sollte nur dann über die 32-Bit Variante emuliert werden, wenn die 64-Bit Variante nicht zur Verfügung steht. Es hat keinen Sinn, auf die 64-Bit Variante zu verzichten nur deswegen, weil die 32-Bit Variante existiert. So wie die Datei derzeit aufgebaut ist, wird der 64-Bit Teil niemals verwendet, weil alle Architekturen den Aufruf statfs haben.

comment:41 Geändert vor 12 Monaten durch er13

In 14480:

busybox:

  • rediff kernel-2.6.28 specific patches
  • refs #2930

comment:42 als Antwort auf: ↑ 40 ; Antwort: Geändert vor 12 Monaten durch er13

Replying to ralf:

So wie die Datei derzeit aufgebaut ist, wird der 64-Bit Teil niemals verwendet, weil alle Architekturen den Aufruf statfs haben.

Kann Dir sehr gut folgen. Ich habe keine Idee, was sich die uClibc-Entwickler dabei gedacht haben könnten (konkret Markos Chandras, der diesen Code in diesem Commit eingeführt hat). Aus meiner Sicht spricht nichts gegen die von Dir vorgeschlagene Änderung.

Dieselbe Logik ist übrigens auch in fstatfs64.c zu finden, vom selben Autor.

Die einzige Erklärung, die ich hätte, ist, dass es Architekturen gibt, auf denen __NR_statfs64 bzw. __NR_fstatfs64 zwar definiert sind, jedoch nicht funktionieren. Deswegen geht man auf Nummer sicher und verwendet die 32-bit Version.

Wenn die Symbole __NR_statfs bzw. __NR_fstatfs immer definiert sind (so wie Du schreibst), so hat eigentlich der gesamte else-Zweig keinen Sinn.

Magst Du auf der uclibc-ng Mailing-Liste nach den eventuellen Hintergründen fragen und ansonsten Deine Änderung auch upstream submitten? Danke!

comment:43 als Antwort auf: ↑ 42 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:

Ich habe keine Idee, was sich die uClibc-Entwickler dabei gedacht haben könnten (konkret Markos Chandras, der diesen Code in diesem Commit eingeführt hat). Aus meiner Sicht spricht nichts gegen die von Dir vorgeschlagene Änderung.

Ich halte es für einen Denkfehler, allerdings hat dann die Änderung auch für ihn keinen Sinn (wie auch für alle anderen). Vielleicht hat er zuerst für sich die Änderung auf Syscall durchgeführt, war mit dem Ergebnis zufrieden, und hat dann das #if eingeführt, aber nicht mehr getestet.

Dieselbe Logik ist übrigens auch in fstatfs64.c zu finden, vom selben Autor.

Das ist keine Überraschung, überraschend ist eher, dass die Änderungen in zwei verschiedenen Commits sind und nicht zusammen.

Die einzige Erklärung, die ich hätte, ist, dass es Architekturen gibt, auf denen __NR_statfs64 bzw. __NR_fstatfs64 zwar definiert sind, jedoch nicht funktionieren. Deswegen geht man auf Nummer sicher und verwendet die 32-bit Version.

Das kann ich nicht ausschließen.

Wenn die Symbole __NR_statfs bzw. __NR_fstatfs immer definiert sind (so wie Du schreibst), so hat eigentlich der gesamte else-Zweig keinen Sinn.

Ich habe jetzt nicht alle Architekturen untersucht, aber x86 hat beide, bei x64 gibt es keine zusätzlich 64-Bit Version, und MIPS hat auch beide. Also alle haben statfs, und x64 hat kein zusätzliches statfs64.

Magst Du auf der uclibc-ng Mailing-Liste nach den eventuellen Hintergründen fragen und ansonsten Deine Änderung auch upstream submitten?

Ich bin auf der uclibc-ng Mailing-Liste nicht drauf, nur bei busybox. Wenn Du willst, kannst Du es gerne schreiben.

Ich habe den Patch mal getestet mit #if !defined __NR_statfs64, und es kommt bei df das richtige Ergebnis. Mit strace sieht man auch, dass statfs64 aufgerufen wird.

comment:44 Geändert vor 12 Monaten durch er13

In 14487:

busybox:

  • update blkid_offset.patch, fixes
    util-linux/volume_id/volume_id.c:111:2: warning: initialization from incompatible pointer type [enabled by default]
      volume_id_probe_bcache,
      ^
    util-linux/volume_id/volume_id.c:111:2: warning: (near initialization for 'fs1[4]') [enabled by default]
    util-linux/volume_id/volume_id.c:172:2: warning: initialization from incompatible pointer type [enabled by default]
      volume_id_probe_ubifs,
      ^
    util-linux/volume_id/volume_id.c:172:2: warning: (near initialization for 'fs2[15]') [enabled by default]
    
  • refs #2930

comment:45 als Antwort auf: ↑ 43 ; Antwort: Geändert vor 12 Monaten durch er13

Replying to ralf:

Die einzige Erklärung, die ich hätte, ist, dass es Architekturen gibt, auf denen __NR_statfs64 bzw. __NR_fstatfs64 zwar definiert sind, jedoch nicht funktionieren. Deswegen geht man auf Nummer sicher und verwendet die 32-bit Version.

Das kann ich nicht ausschließen.

Das war wohl früher der Fall. alpha hat bis zu diesem Commit keine eigene Version von unistd.h gehabt, was dazu geführt hat, dass das Symbol __NR_statfs64 definiert wurde, der Aufruf des entsprechenden Syscalls jedoch ENOSYS geliefert hat. S. auch sonst die Anzahl der not implemented.

comment:46 als Antwort auf: ↑ 45 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:

alpha hat bis zu diesem Commit keine eigene Version von unistd.h gehabt, was dazu geführt hat, dass das Symbol __NR_statfs64 definiert wurde, der Aufruf des entsprechenden Syscalls jedoch ENOSYS geliefert hat.

Nun, Alpha ist nicht unser Problem, und ich betrachte das als einen Fehler im Kernel, der inzwischen korrigiert ist. Es ändert nichts daran, dass es keine Sinn hat, etwas in den else Zweig zu schreiben, wenn __NR_statfs vermutlich immer definiert ist. Es ist dann besser, sich auf die Definition vom Kernel zu stützen, und wo die nicht korrekt ist, kann man immer noch in eigenen Architektur abhängigen Dateien das anpassen.

Ich habe inzwischen auf der Busybox Liste geschrieben, was man an uClibc ändern kann, ich schlage vor, dass wir diese Änderungen für Freetz übernehmen.

comment:47 als Antwort auf: ↑ 46 Geändert vor 12 Monaten durch er13

Replying to ralf:

ich schlage vor, dass wir diese Änderungen für Freetz übernehmen.

Habe nichts dagegen. Die erste Änderung (#define _STATFS_F_FRSIZE ) ist seit r14474 enthalten, bei der zweiten bitte auch an fstatfs64.c denken.

comment:48 Geändert vor 12 Monaten durch ralf

In 14498:

  • uClibc: use fstatfs64/statfs64 syscalls instead of emulation refs #2930

comment:49 Antwort: Geändert vor 12 Monaten durch ralf

In 14500:

comment:50 als Antwort auf: ↑ 49 ; Antwort: Geändert vor 12 Monaten durch er13

Replying to ralf:

In 14500:

Warum befindet sich #if !defined __NR_... in einem Fall VOR und in dem anderen NACH libc_hidden_proto? Flüchtigkeitsfehler oder was dabei gedacht?

comment:51 Geändert vor 12 Monaten durch ralf

In 14502:

  • uClibc: fix statfs64 patch for older versions refs #2930

comment:52 als Antwort auf: ↑ 50 ; Antwort: Geändert vor 12 Monaten durch ralf

Replying to er13:
Keine Absicht. Das #if vor libc_hidden_proto wäre sauberer, aber es sind nur Deklarationen, daher macht es keinen Unterschied.

Die alten Libraries haben auch kein INLINE_SYSCALL. Jetzt kann man auf jeden Fall eine Firmware damit erstellen, bei Gelegenheit werde ich sie auf einer 3170 testen. Wenn ich zusätzlich zu fast allen Remove Patches noch ext2.ko entferne, kann ich sogar ein 7170→3170 Alien erstellen. Mit 3170 und 7170→3170 ist uClibc 0.9.28 und 0.9.29 abgedeckt.

uClibc 0.9.32.1 und binutils 2.22 zusammen funktioniert übrigens nicht, der Assembler kennt -EL nicht. Da wir sowieso den zur Box passenden Assembler erstellen, ist der Default schon korrekt und die Option wäre nicht notwendig.

comment:53 als Antwort auf: ↑ 52 ; Antwort: Geändert vor 12 Monaten durch er13

Replying to ralf:

uClibc 0.9.32.1 und binutils 2.22 zusammen funktioniert übrigens nicht, der Assembler kennt -EL nicht. Da wir sowieso den zur Box passenden Assembler erstellen, ist der Default schon korrekt und die Option wäre nicht notwendig.

Habe diesen Teil nicht verstanden. Könntest Du es bitte nochmal erklären?

comment:54 als Antwort auf: ↑ 53 Geändert vor 12 Monaten durch ralf

Replying to er13:

Replying to ralf:

uClibc 0.9.32.1 und binutils 2.22 zusammen funktioniert übrigens nicht, der Assembler kennt -EL nicht. Da wir sowieso den zur Box passenden Assembler erstellen, ist der Default schon korrekt und die Option wäre nicht notwendig.

Habe diesen Teil nicht verstanden. Könntest Du es bitte nochmal erklären?

Wenn ich im menuconfig bei "Toolchain options" "Target uClibc version 0.9.32.1" auswähle und "Target binutils binutils-2.23.2", dann gibt es beim Erstellen von uclibc den Fehler, dass der Assembler die Option -EL nicht kennt. Ich habe aber festgestellt, dass der Host-Assmebler aufgerufen wird und nicht der Cross-Assembler, und deswegen die Option unbekannt ist. Wenn man binutils neu erstellt, ist der Fehler weg. Von daher vermute ich, dass es nichts mit der Version von binutils zu tun hat, sondern mit fehlenden Abhängigkeiten, die dazu führen, dass beim Wechsel der uClibc Version die binutils nicht ins passende Verzeichnis kopiert werden.

In dem Zusammenhang, was ist die Unterscheidung zwischen source/toolchain-mipsel_gcc-4.6.4_uClibc-0.9.29/uClibc_dev und toolchain/build/mipsel_gcc-4.6.4_uClibc-0.9.29/mipsel-linux-uclibc, bzw. warum die Trennung zwischen den beiden? Im Prinzip enthalten beide Teile von dem, was bei GCC sysroot genannt wird, die Include-Dateien und ausführbaren Programme für die Ziel-Architektur.

comment:55 Geändert vor 12 Monaten durch ralf

In 14506:

  • uclibc: fir statfs64 syscall refs #2930

comment:56 Geändert vor 11 Monaten durch er13

In 14517:

busybox, unzip-applet:

  • backport some upstream patches
  • replace patch added in r14513 with its more recent version
  • refs #2930

comment:57 Geändert vor 5 Monaten durch er13

  • Lösung auf fixed gesetzt
  • Status von new nach closed geändert
Hinweis: Hilfe zur Verwendung von Tickets finden Sie in TracTickets.