| Message ID | a9431732555c1de953667ffc339acd8655504323.1688396342.git.nabijaczleweli@nabijaczleweli.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] hurd: statvfs: __f_type -> f_type | expand |
On 03/07/23 11:59, наб via Libc-alpha wrote: > Also fix tst-statvfs so that it actually fails; > as it stood, all it did was return 0 always. > > Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> I think this patch should be squashed with previous one, rest looks ok. > --- > NEWS | 5 +++++ > io/tst-statvfs.c | 19 +++++++++++-------- > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/NEWS b/NEWS > index 709ee40e50..fc2392f168 100644 > --- a/NEWS > +++ b/NEWS > @@ -48,6 +48,11 @@ Major new features: > * The strlcpy and strlcat functions have been added. They are derived > from OpenBSD, and are expected to be added to a future POSIX version. > > +* struct statvfs now has an f_type member, equal to the f_type statfs member; > + on the Hurd this was always available under a reserved name, > + and under Linux a spare has been allocated: it was always zero > + in previous versions of glibc, and zero is not a valid result. > + > Deprecated and removed features, and other changes affecting compatibility: > > * In the Linux kernel for the hppa/parisc architecture some of the > diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c > index 227c62d7da..b38ecae466 100644 > --- a/io/tst-statvfs.c > +++ b/io/tst-statvfs.c > @@ -1,5 +1,7 @@ > #include <stdio.h> > +#include <sys/statfs.h> > #include <sys/statvfs.h> > +#include <support/check.h> > > > /* This test cannot detect many errors. But it will fail if the > @@ -11,17 +13,18 @@ do_test (int argc, char *argv[]) > for (int i = 1; i < argc; ++i) > { > struct statvfs st; > - if (statvfs (argv[i], &st) != 0) > - printf ("%s: failed (%m)\n", argv[i]); > - else > - printf ("%s: free: %llu, mandatory: %s\n", argv[i], > - (unsigned long long int) st.f_bfree, > + struct statfs stf; > + TEST_COMPARE (statvfs (argv[i], &st), 0); > + TEST_COMPARE (statfs (argv[i], &stf), 0); > + TEST_COMPARE (st.f_type, stf.f_type); > + printf ("%s: free: %llu, mandatory: %s\n", argv[i], > + (unsigned long long int) st.f_bfree, > #ifdef ST_MANDLOCK > - (st.f_flag & ST_MANDLOCK) ? "yes" : "no" > + (st.f_flag & ST_MANDLOCK) ? "yes" : "no" > #else > - "no" > + "no" > #endif > - ); > + ); > } > return 0; > }
* наб: > diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c > index 227c62d7da..b38ecae466 100644 > --- a/io/tst-statvfs.c > +++ b/io/tst-statvfs.c > @@ -1,5 +1,7 @@ > #include <stdio.h> > +#include <sys/statfs.h> > #include <sys/statvfs.h> > +#include <support/check.h> > > > /* This test cannot detect many errors. But it will fail if the > @@ -11,17 +13,18 @@ do_test (int argc, char *argv[]) > for (int i = 1; i < argc; ++i) > { > struct statvfs st; > - if (statvfs (argv[i], &st) != 0) > - printf ("%s: failed (%m)\n", argv[i]); > - else > - printf ("%s: free: %llu, mandatory: %s\n", argv[i], > - (unsigned long long int) st.f_bfree, > + struct statfs stf; > + TEST_COMPARE (statvfs (argv[i], &st), 0); > + TEST_COMPARE (statfs (argv[i], &stf), 0); > + TEST_COMPARE (st.f_type, stf.f_type); This fails with certain file systems because the types of f_type differ in signedness: =====FAIL: io/tst-statvfs.out===== tst-statvfs.c:19: numeric comparison failure left: 2435016766 (0x9123683e); from: st.f_type right: -1859950530 (0x9123683e); from: stf.f_type /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/ tst-statvfs: free: 53658025, mandatory: no, tp=9123683e tst-statvfs.c:19: numeric comparison failure left: 2435016766 (0x9123683e); from: st.f_type right: -1859950530 (0x9123683e); from: stf.f_type tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e tst-statvfs.c:19: numeric comparison failure left: 2435016766 (0x9123683e); from: st.f_type right: -1859950530 (0x9123683e); from: stf.f_type /tmp: free: 53658025, mandatory: no, tp=9123683e error: 3 test failures Thanks, Florian
On 15/08/23 06:15, Florian Weimer wrote: > * наб: > >> diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c >> index 227c62d7da..b38ecae466 100644 >> --- a/io/tst-statvfs.c >> +++ b/io/tst-statvfs.c >> @@ -1,5 +1,7 @@ >> #include <stdio.h> >> +#include <sys/statfs.h> >> #include <sys/statvfs.h> >> +#include <support/check.h> >> >> >> /* This test cannot detect many errors. But it will fail if the >> @@ -11,17 +13,18 @@ do_test (int argc, char *argv[]) >> for (int i = 1; i < argc; ++i) >> { >> struct statvfs st; >> - if (statvfs (argv[i], &st) != 0) >> - printf ("%s: failed (%m)\n", argv[i]); >> - else >> - printf ("%s: free: %llu, mandatory: %s\n", argv[i], >> - (unsigned long long int) st.f_bfree, >> + struct statfs stf; >> + TEST_COMPARE (statvfs (argv[i], &st), 0); >> + TEST_COMPARE (statfs (argv[i], &stf), 0); >> + TEST_COMPARE (st.f_type, stf.f_type); > > This fails with certain file systems because the types of f_type differ > in signedness: > > =====FAIL: io/tst-statvfs.out===== > tst-statvfs.c:19: numeric comparison failure > left: 2435016766 (0x9123683e); from: st.f_type > right: -1859950530 (0x9123683e); from: stf.f_type > /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/ > tst-statvfs: free: 53658025, mandatory: no, tp=9123683e > tst-statvfs.c:19: numeric comparison failure > left: 2435016766 (0x9123683e); from: st.f_type > right: -1859950530 (0x9123683e); from: stf.f_type > tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e > tst-statvfs.c:19: numeric comparison failure > left: 2435016766 (0x9123683e); from: st.f_type > right: -1859950530 (0x9123683e); from: stf.f_type > /tmp: free: 53658025, mandatory: no, tp=9123683e > error: 3 test failures Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option.
* Adhemerval Zanella Netto: >> This fails with certain file systems because the types of f_type differ >> in signedness: >> >> =====FAIL: io/tst-statvfs.out===== >> tst-statvfs.c:19: numeric comparison failure >> left: 2435016766 (0x9123683e); from: st.f_type >> right: -1859950530 (0x9123683e); from: stf.f_type >> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/ >> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e >> tst-statvfs.c:19: numeric comparison failure >> left: 2435016766 (0x9123683e); from: st.f_type >> right: -1859950530 (0x9123683e); from: stf.f_type >> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e >> tst-statvfs.c:19: numeric comparison failure >> left: 2435016766 (0x9123683e); from: st.f_type >> right: -1859950530 (0x9123683e); from: stf.f_type >> /tmp: free: 53658025, mandatory: no, tp=9123683e >> error: 3 test failures > > Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option. Or change the type of the new field to match the old field? Due to the way TEST_COMPARE works, a no-sign option does not make much sense, I'm afraid. It's supposed to compare the mathematical values regardless of type sizes. If we want to accept certain signed vs unsigned mismatches as valid, I think we need to use a common size, at which point we might as well use a cast. Conceptually, this should be close to what we want: if (st.f_type != stf.f_type) TEST_COMPARE (st.f_type, stf.f_type); Except that I expect it to produce a compiler warning about signedness mismatch. So yes, I think we're going to have to add a cast. Thanks, Florian
On 15/08/23 09:41, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >>> This fails with certain file systems because the types of f_type differ >>> in signedness: >>> >>> =====FAIL: io/tst-statvfs.out===== >>> tst-statvfs.c:19: numeric comparison failure >>> left: 2435016766 (0x9123683e); from: st.f_type >>> right: -1859950530 (0x9123683e); from: stf.f_type >>> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/ >>> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e >>> tst-statvfs.c:19: numeric comparison failure >>> left: 2435016766 (0x9123683e); from: st.f_type >>> right: -1859950530 (0x9123683e); from: stf.f_type >>> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e >>> tst-statvfs.c:19: numeric comparison failure >>> left: 2435016766 (0x9123683e); from: st.f_type >>> right: -1859950530 (0x9123683e); from: stf.f_type >>> /tmp: free: 53658025, mandatory: no, tp=9123683e >>> error: 3 test failures >> >> Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option. > > Or change the type of the new field to match the old field? We discussed this briefly [1] on why Ahelenia decided to use an unsigned type. We still can change it, I don't have a strong opinion here. > > Due to the way TEST_COMPARE works, a no-sign option does not make much > sense, I'm afraid. It's supposed to compare the mathematical values > regardless of type sizes. If we want to accept certain signed vs > unsigned mismatches as valid, I think we need to use a common size, at > which point we might as well use a cast. > > Conceptually, this should be close to what we want: > > if (st.f_type != stf.f_type) > TEST_COMPARE (st.f_type, stf.f_type); > > Except that I expect it to produce a compiler warning about signedness > mismatch. So yes, I think we're going to have to add a cast. > > Thanks, > Florian > [1] https://sourceware.org/pipermail/libc-alpha/2023-July/150308.html
On Tue, Aug 15, 2023 at 02:41:56PM +0200, Florian Weimer wrote: > * Adhemerval Zanella Netto: > > >> This fails with certain file systems because the types of f_type differ > >> in signedness: > >> > >> =====FAIL: io/tst-statvfs.out===== > >> tst-statvfs.c:19: numeric comparison failure > >> left: 2435016766 (0x9123683e); from: st.f_type > >> right: -1859950530 (0x9123683e); from: stf.f_type > >> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/ > >> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e > >> tst-statvfs.c:19: numeric comparison failure > >> left: 2435016766 (0x9123683e); from: st.f_type > >> right: -1859950530 (0x9123683e); from: stf.f_type > >> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e > >> tst-statvfs.c:19: numeric comparison failure > >> left: 2435016766 (0x9123683e); from: st.f_type > >> right: -1859950530 (0x9123683e); from: stf.f_type > >> /tmp: free: 53658025, mandatory: no, tp=9123683e > >> error: 3 test failures > > > > Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option. > > Or change the type of the new field to match the old field? See 92861d93cdad13834f4d8f39504b550a80ad8200 for why it's an u32 (in short: the values are all logically u32s, but defined as untyped 0x123 literals by accident, C++ needs this to be u32 to allow switch/casing on *_MAGICs, the ABI for struct statfs uses an effectively-random (signed/unsigned 32/64-bit integer) unnameable type that depends on the architecture, so we provide a sane and unmisuable API to that). To that last point ‒ that test failure doesn't happen on amd64, even with btrfs like in your log, only on i686. (I'd be shocked if current struct statfs{}.f_type users were all correct on all arches in the era of amd64 monoculture; I knew about this and still wrote it wrong!) > Due to the way TEST_COMPARE works, a no-sign option does not make much > sense, I'm afraid. It's supposed to compare the mathematical values > regardless of type sizes. If we want to accept certain signed vs > unsigned mismatches as valid, I think we need to use a common size, at > which point we might as well use a cast. Making the line "TEST_COMPARE (st.f_type, (unsigned int) stf.f_type);" did fix it in an i686 chroot. idk if it's easier for you to take a patch or just edit the file, but in case it's the former, scissor-patch follows. Best, -- >8 -- Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some arches On i686 f_type is an i32 so the test fails when that has the top bit set. Explicitly cast to u32. Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> --- io/tst-statvfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c index f3097ce1a8..fb23733667 100644 --- a/io/tst-statvfs.c +++ b/io/tst-statvfs.c @@ -16,7 +16,7 @@ do_test (int argc, char *argv[]) struct statfs stf; TEST_COMPARE (statvfs (argv[i], &st), 0); TEST_COMPARE (statfs (argv[i], &stf), 0); - TEST_COMPARE (st.f_type, stf.f_type); + TEST_COMPARE (st.f_type, (unsigned int) stf.f_type); printf ("%s: free: %llu, mandatory: %s, tp=%x\n", argv[i], (unsigned long long int) st.f_bfree, #ifdef ST_MANDLOCK
* наб: > Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some > arches > > On i686 f_type is an i32 so the test fails when that has the top bit set. > > Explicitly cast to u32. > > Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> > --- > io/tst-statvfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c > index f3097ce1a8..fb23733667 100644 > --- a/io/tst-statvfs.c > +++ b/io/tst-statvfs.c > @@ -16,7 +16,7 @@ do_test (int argc, char *argv[]) > struct statfs stf; > TEST_COMPARE (statvfs (argv[i], &st), 0); > TEST_COMPARE (statfs (argv[i], &stf), 0); > - TEST_COMPARE (st.f_type, stf.f_type); > + TEST_COMPARE (st.f_type, (unsigned int) stf.f_type); > printf ("%s: free: %llu, mandatory: %s, tp=%x\n", argv[i], > (unsigned long long int) st.f_bfree, > #ifdef ST_MANDLOCK Looks good. Reviewed-by: Florian Weimer <fweimer@redhat.com> Can you install this yourself, or should I push it? Thanks, Florian
On Tue, Aug 15, 2023 at 04:23:37PM +0200, Florian Weimer wrote: > * наб: > > Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some > > arches > Looks good. > > Reviewed-by: Florian Weimer <fweimer@redhat.com> > > Can you install this yourself, Don't know what this means, sorry; I don't think I have write access to the glibc repo? > or should I push it? Please do. Best,
* наб: > On Tue, Aug 15, 2023 at 04:23:37PM +0200, Florian Weimer wrote: >> * наб: >> > Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some >> > arches >> Looks good. >> >> Reviewed-by: Florian Weimer <fweimer@redhat.com> >> >> Can you install this yourself, > Don't know what this means, sorry; > I don't think I have write access to the glibc repo? That's what I meant. >> or should I push it? > Please do. Pushed it. Thanks, Florian
diff --git a/NEWS b/NEWS index 709ee40e50..fc2392f168 100644 --- a/NEWS +++ b/NEWS @@ -48,6 +48,11 @@ Major new features: * The strlcpy and strlcat functions have been added. They are derived from OpenBSD, and are expected to be added to a future POSIX version. +* struct statvfs now has an f_type member, equal to the f_type statfs member; + on the Hurd this was always available under a reserved name, + and under Linux a spare has been allocated: it was always zero + in previous versions of glibc, and zero is not a valid result. + Deprecated and removed features, and other changes affecting compatibility: * In the Linux kernel for the hppa/parisc architecture some of the diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c index 227c62d7da..b38ecae466 100644 --- a/io/tst-statvfs.c +++ b/io/tst-statvfs.c @@ -1,5 +1,7 @@ #include <stdio.h> +#include <sys/statfs.h> #include <sys/statvfs.h> +#include <support/check.h> /* This test cannot detect many errors. But it will fail if the @@ -11,17 +13,18 @@ do_test (int argc, char *argv[]) for (int i = 1; i < argc; ++i) { struct statvfs st; - if (statvfs (argv[i], &st) != 0) - printf ("%s: failed (%m)\n", argv[i]); - else - printf ("%s: free: %llu, mandatory: %s\n", argv[i], - (unsigned long long int) st.f_bfree, + struct statfs stf; + TEST_COMPARE (statvfs (argv[i], &st), 0); + TEST_COMPARE (statfs (argv[i], &stf), 0); + TEST_COMPARE (st.f_type, stf.f_type); + printf ("%s: free: %llu, mandatory: %s\n", argv[i], + (unsigned long long int) st.f_bfree, #ifdef ST_MANDLOCK - (st.f_flag & ST_MANDLOCK) ? "yes" : "no" + (st.f_flag & ST_MANDLOCK) ? "yes" : "no" #else - "no" + "no" #endif - ); + ); } return 0; }
Also fix tst-statvfs so that it actually fails; as it stood, all it did was return 0 always. Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> --- NEWS | 5 +++++ io/tst-statvfs.c | 19 +++++++++++-------- 2 files changed, 16 insertions(+), 8 deletions(-)