| Message ID | 20210802172116.10073-1-petr.vorel@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] package/nfs-utils: Backport yet another printf fix | expand |
Hi Petr, > Il giorno 2 ago 2021, alle ore 19:21, Petr Vorel <petr.vorel@gmail.com> ha scritto: > > Previous fixes from daa5459b6a caused regression that even more builds > were broken. Backporting upstream fix 383d787d ("nfsdcltrack: Use > uint64_t instead of time_t") which fixes it. > It fixes bug: > nfsdcltrack.c: In function ‘cltrack_gracedone’: > nfsdcltrack.c:529:18: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘time_t’ {aka ‘long int’} [-Werror=format=] > 529 | xlog(D_GENERAL, "%s: grace done. gracetime=%"PRIu64, __func__, gracetime); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~ > | | > | time_t {aka long int} > > Fixes: > - http://autobuild.buildroot.net/results/550ad9c74666775858887050ea83a8618797de79 > - http://autobuild.buildroot.net/results/caf0a69121d49504db63910ce1df347802065cd9 > - http://autobuild.buildroot.net/results/294d11f8c4330bcfd9412f47f21df22d4f2b6e82 > - http://autobuild.buildroot.net/results/1cb6b19cbbc2b829a038a9a70b6734c084e81c93 > - http://autobuild.buildroot.net/results/c0eb120f5d14798b4f88b90eaccd3bc1f657c00d > - http://autobuild.buildroot.net/results/a028299b612a0ca6d1a69757749a8b9e37094954 > - http://autobuild.buildroot.net/results/996bb74445cfbde0e230b2185207d91d4ec46dfb > - http://autobuild.buildroot.net/results/624b6dc0793d0263f46831ed0f29fa265ff0e41f > - http://autobuild.buildroot.net/results/5eb2dd5d6bc0540443a52f0b9b9568af01892a23 > - http://autobuild.buildroot.net/results/65bad834738d5c5b1176e8847777b3cfd95c4351 > - http://autobuild.buildroot.net/results/3146237c356e5649163784fb6628fbbbc7c61153 > - http://autobuild.buildroot.net/results/2c5e77600cbf5009e69f057e7a2dc164fc8b0466 > - http://autobuild.buildroot.net/results/ec8ec029dc0d7d7d294f8ade81ee53ca2fc3c54c > - http://autobuild.buildroot.net/results/60ea3bdeddf71270468d6177ccf95a1dc6b68dac > - http://autobuild.buildroot.net/results/ff93a63cd084043db5f61ac5ea2bbbca4f0e6859 > - http://autobuild.buildroot.net/results/cac3392373d05d78c299d07d8080ae4938773401 > - http://autobuild.buildroot.net/results/438155f831d8a44ec6e0f7020e8783b48d2c8df2 > - http://autobuild.buildroot.net/results/66bc1ca1a05e1676c4e65ef7364ba1415bdbc723 > - http://autobuild.buildroot.net/results/5bacb3c38dc1012f9445a0c23e59336dcac5abce > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> Reviewed-by: Giulio Benetti <giulio.benetti@benettiengineering.com> And thank you for fixing this :-) Best regards Giulio Benetti > --- > I'm sorry for not catching these. Our testing tools didn't caught that > (probably was not using -Werror). > > Tested: > bootlin-armv5-uclibc [1/6]: OK > bootlin-armv7-glibc [2/6]: OK > bootlin-armv7m-uclibc [3/6]: SKIPPED > bootlin-x86-64-musl [4/6]: OK > br-arm-full-static [5/6]: OK > sourcery-arm [6/6]: OK > 6 builds, 1 skipped, 0 build failed, 0 legal-info failed > > andes-nds32 [ 1/45]: OK > arm-aarch64 [ 2/45]: OK > bootlin-aarch64-glibc [ 3/45]: OK > bootlin-arcle-hs38-uclibc [ 4/45]: OK > bootlin-armv5-uclibc [ 5/45]: OK > bootlin-armv7-glibc [ 6/45]: OK > bootlin-armv7m-uclibc [ 7/45]: SKIPPED > bootlin-armv7-musl [ 8/45]: OK > bootlin-microblazeel-uclibc [ 9/45]: OK > bootlin-mipsel-uclibc [10/45]: OK > bootlin-mipsel32r6-glibc [11/45]: OK > bootlin-m68k-5208-uclibc [12/45]: SKIPPED > bootlin-m68k-68040-uclibc [13/45]: OK > bootlin-nios2-glibc [14/45]: OK > bootlin-openrisc-uclibc [15/45]: OK > bootlin-powerpc-e500mc-uclibc [16/45]: OK > bootlin-powerpc64le-power8-glibc [17/45]: OK > bootlin-riscv32-glibc [18/45]: OK > bootlin-riscv64-glibc [19/45]: OK > bootlin-riscv64-musl [20/45]: OK > bootlin-sh4-uclibc [21/45]: OK > bootlin-sparc-uclibc [22/45]: OK > bootlin-sparc64-glibc [23/45]: OK > bootlin-xtensa-uclibc [24/45]: OK > bootlin-x86-64-glibc [25/45]: OK > bootlin-x86-64-musl [26/45]: OK > bootlin-x86-64-uclibc [27/45]: OK > br-arm-basic [28/45]: OK > br-arm-full-nothread [29/45]: SKIPPED > br-arm-full-static [30/45]: OK > br-i386-pentium-mmx-musl [31/45]: OK > br-i386-pentium4-full [32/45]: OK > br-mips64-n64-full [33/45]: OK > br-mips64r6-el-hf-glibc [34/45]: OK > br-powerpc-603e-basic-cpp [35/45]: OK > br-powerpc64-power7-glibc [36/45]: OK > > Kind regards, > Petr > > ...track-Use-uint64_t-instead-of-time_t.patch | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch > > diff --git a/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch b/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch > new file mode 100644 > index 0000000000..384f4fd806 > --- /dev/null > +++ b/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch > @@ -0,0 +1,66 @@ > +From 383d787d1b77f165da68495cb0363220b66935a4 Mon Sep 17 00:00:00 2001 > +From: Steve Dickson <steved@redhat.com> > +Date: Tue, 27 Jul 2021 21:12:17 -0400 > +Subject: [PATCH] nfsdcltrack: Use uint64_t instead of time_t > + > +With recent commits (4f2a5b64,5a53426c) that fixed > +compile errors on x86_64 machines, caused similar > +errors on i686 machines. > + > +The variable type that was being used was a time_t, > +which changes size between architects, which > +caused the compile error. > + > +Changing the variable to uint64_t fixed the issue. > + > +Signed-off-by: Steve Dickson <steved@redhat.com> > +Upstream: http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=commit;h=383d787d1b77f165da68495cb0363220b66935a4 > +Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > +--- > + utils/nfsdcltrack/nfsdcltrack.c | 2 +- > + utils/nfsdcltrack/sqlite.c | 2 +- > + utils/nfsdcltrack/sqlite.h | 2 +- > + 3 files changed, 3 insertions(+), 3 deletions(-) > + > +diff --git a/utils/nfsdcltrack/nfsdcltrack.c b/utils/nfsdcltrack/nfsdcltrack.c > +index 0b37c094..7c1c4bcc 100644 > +--- a/utils/nfsdcltrack/nfsdcltrack.c > ++++ b/utils/nfsdcltrack/nfsdcltrack.c > +@@ -508,7 +508,7 @@ cltrack_gracedone(const char *timestr) > + { > + int ret; > + char *tail; > +- time_t gracetime; > ++ uint64_t gracetime; > + > + > + ret = sqlite_prepare_dbh(storagedir); > +diff --git a/utils/nfsdcltrack/sqlite.c b/utils/nfsdcltrack/sqlite.c > +index cea4a411..cf0c6a45 100644 > +--- a/utils/nfsdcltrack/sqlite.c > ++++ b/utils/nfsdcltrack/sqlite.c > +@@ -540,7 +540,7 @@ out_err: > + * remove any client records that were not reclaimed since grace_start. > + */ > + int > +-sqlite_remove_unreclaimed(time_t grace_start) > ++sqlite_remove_unreclaimed(uint64_t grace_start) > + { > + int ret; > + char *err = NULL; > +diff --git a/utils/nfsdcltrack/sqlite.h b/utils/nfsdcltrack/sqlite.h > +index 06e7c044..ba8cdfa8 100644 > +--- a/utils/nfsdcltrack/sqlite.h > ++++ b/utils/nfsdcltrack/sqlite.h > +@@ -26,7 +26,7 @@ int sqlite_insert_client(const unsigned char *clname, const size_t namelen, > + int sqlite_remove_client(const unsigned char *clname, const size_t namelen); > + int sqlite_check_client(const unsigned char *clname, const size_t namelen, > + const bool has_session); > +-int sqlite_remove_unreclaimed(const time_t grace_start); > ++int sqlite_remove_unreclaimed(const uint64_t grace_start); > + int sqlite_query_reclaiming(const time_t grace_start); > + > + #endif /* _SQLITE_H */ > +-- > +2.32.0 > + > -- > 2.32.0 >
Hi Giulio, > Reviewed-by: Giulio Benetti <giulio.benetti@benettiengineering.com> > And thank you for fixing this :-) Yw. I see time_t printf format is more tricky than I expected, your traditional approach with casting to long long would not require this change to uint64_t :). Kind regards, Petr > Best regards > Giulio Benetti
On 8/2/21 9:17 PM, Petr Vorel wrote: > Hi Giulio, > >> Reviewed-by: Giulio Benetti <giulio.benetti@benettiengineering.com> > >> And thank you for fixing this :-) > Yw. I see time_t printf format is more tricky than I expected, really! > your traditional > approach with casting to long long would not require this change to uint64_t :). Yes, but Steve preferred your way that is more portable. ;-) Kind regards
On Mon, 2 Aug 2021 21:17:12 +0200 Petr Vorel <petr.vorel@gmail.com> wrote: > > And thank you for fixing this :-) > Yw. I see time_t printf format is more tricky than I expected, your traditional > approach with casting to long long would not require this change to uint64_t :). Actually, we have this printf() / time_t issue on other packages, on 32-bit architectures where time_t is a 64-bit value (RISC-V 32-bit for example). What is the correct solution in the end? Apparently, the really standard solution is simply to *not* print a time_t, as it's supposed to be an opaque type, which can be different things depending on the underlying implementation. But in practice, a lot of packages do printf() time_t values. Thomas
Hi Thomas, > On Mon, 2 Aug 2021 21:17:12 +0200 > Petr Vorel <petr.vorel@gmail.com> wrote: > > > And thank you for fixing this :-) > > Yw. I see time_t printf format is more tricky than I expected, your traditional > > approach with casting to long long would not require this change to uint64_t :). > Actually, we have this printf() / time_t issue on other packages, on > 32-bit architectures where time_t is a 64-bit value (RISC-V 32-bit for > example). What is the correct solution in the end? I believe all 3 patches (2 already merged + this one) should fix the problem. But I was not able to verify it, because ./utils/test-pkg didn't catch even this error (I tested all available toolchains), IMHO -Werror=format=2 and other -Werror are probably only on http://autobuild.buildroot.net/ (not in Buildroot config for users). It'd be great if ./utils/test-pkg had the same CFLAGS. Or have I (again) overlooked something? > Apparently, the really standard solution is simply to *not* print a > time_t, as it's supposed to be an opaque type, which can be different > things depending on the underlying implementation. But in practice, a > lot of packages do printf() time_t values. Yep, convert it to string is recommended, but as you noted, many people do. > Thomas Kind regards, Petr
On Tue, 3 Aug 2021 00:06:46 +0200 Petr Vorel <petr.vorel@gmail.com> wrote: > I believe all 3 patches (2 already merged + this one) should fix the problem. > But I was not able to verify it, because ./utils/test-pkg didn't catch even > this error (I tested all available toolchains), IMHO -Werror=format=2 and other > -Werror are probably only on http://autobuild.buildroot.net/ (not in Buildroot > config for users). It'd be great if ./utils/test-pkg had the same CFLAGS. > Or have I (again) overlooked something? No, the autobuilders don't do anything specific with -Werror CFLAGS. The configurations tested by the autobuilders are generated by utils/genrandconfig in the Buildroot tree. Besides the obvious package randomization, there is some randomization of "global" options: # Per-package folder if randint(0, 15) == 0: configlines.append("BR2_PER_PACKAGE_DIRECTORIES=y\n") # Amend the configuration with a few things. if randint(0, 20) == 0: configlines.append("BR2_ENABLE_DEBUG=y\n") if randint(0, 20) == 0: configlines.append("BR2_ENABLE_RUNTIME_DEBUG=y\n") if randint(0, 1) == 0: configlines.append("BR2_INIT_BUSYBOX=y\n") elif randint(0, 15) == 0: configlines.append("BR2_INIT_SYSTEMD=y\n") elif randint(0, 10) == 0: configlines.append("BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y\n") if randint(0, 20) == 0: configlines.append("BR2_STATIC_LIBS=y\n") if randint(0, 20) == 0: configlines.append("BR2_PACKAGE_PYTHON_PY_ONLY=y\n") if randint(0, 5) == 0: configlines.append("BR2_OPTIMIZE_2=y\n") if randint(0, 4) == 0: configlines.append("BR2_SYSTEM_ENABLE_NLS=y\n") if randint(0, 4) == 0: configlines.append("BR2_FORTIFY_SOURCE_2=y\n") For example, did you test with BR2_FORTIFY_SOURCE_2=y ? Could you point me to the autobuilder failure that you had, but was not able to reproduce with test-pkg ? We should be able to point out the difference. The thing is that test-pkg cannot test all possibilities, it would take way too much time. Thomas
02.08.2021 20:21, Petr Vorel wrote: > Previous fixes from daa5459b6a caused regression that even more builds > were broken. Backporting upstream fix 383d787d ("nfsdcltrack: Use > uint64_t instead of time_t") which fixes it. > It fixes bug: > nfsdcltrack.c: In function ‘cltrack_gracedone’: > nfsdcltrack.c:529:18: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘time_t’ {aka ‘long int’} [-Werror=format=] > 529 | xlog(D_GENERAL, "%s: grace done. gracetime=%"PRIu64, __func__, gracetime); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~ > | | > | time_t {aka long int} > > Fixes: > - http://autobuild.buildroot.net/results/550ad9c74666775858887050ea83a8618797de79 > - http://autobuild.buildroot.net/results/caf0a69121d49504db63910ce1df347802065cd9 > - http://autobuild.buildroot.net/results/294d11f8c4330bcfd9412f47f21df22d4f2b6e82 > - http://autobuild.buildroot.net/results/1cb6b19cbbc2b829a038a9a70b6734c084e81c93 > - http://autobuild.buildroot.net/results/c0eb120f5d14798b4f88b90eaccd3bc1f657c00d > - http://autobuild.buildroot.net/results/a028299b612a0ca6d1a69757749a8b9e37094954 > - http://autobuild.buildroot.net/results/996bb74445cfbde0e230b2185207d91d4ec46dfb > - http://autobuild.buildroot.net/results/624b6dc0793d0263f46831ed0f29fa265ff0e41f > - http://autobuild.buildroot.net/results/5eb2dd5d6bc0540443a52f0b9b9568af01892a23 > - http://autobuild.buildroot.net/results/65bad834738d5c5b1176e8847777b3cfd95c4351 > - http://autobuild.buildroot.net/results/3146237c356e5649163784fb6628fbbbc7c61153 > - http://autobuild.buildroot.net/results/2c5e77600cbf5009e69f057e7a2dc164fc8b0466 > - http://autobuild.buildroot.net/results/ec8ec029dc0d7d7d294f8ade81ee53ca2fc3c54c > - http://autobuild.buildroot.net/results/60ea3bdeddf71270468d6177ccf95a1dc6b68dac > - http://autobuild.buildroot.net/results/ff93a63cd084043db5f61ac5ea2bbbca4f0e6859 > - http://autobuild.buildroot.net/results/cac3392373d05d78c299d07d8080ae4938773401 > - http://autobuild.buildroot.net/results/438155f831d8a44ec6e0f7020e8783b48d2c8df2 > - http://autobuild.buildroot.net/results/66bc1ca1a05e1676c4e65ef7364ba1415bdbc723 > - http://autobuild.buildroot.net/results/5bacb3c38dc1012f9445a0c23e59336dcac5abce > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> Tested-by: Maxim Kochetkov <fido_max@inbox.ru>
Hi Thomas, > On Tue, 3 Aug 2021 00:06:46 +0200 > Petr Vorel <petr.vorel@gmail.com> wrote: > > I believe all 3 patches (2 already merged + this one) should fix the problem. > > But I was not able to verify it, because ./utils/test-pkg didn't catch even > > this error (I tested all available toolchains), IMHO -Werror=format=2 and other > > -Werror are probably only on http://autobuild.buildroot.net/ (not in Buildroot > > config for users). It'd be great if ./utils/test-pkg had the same CFLAGS. > > Or have I (again) overlooked something? > No, the autobuilders don't do anything specific with -Werror CFLAGS. > The configurations tested by the autobuilders are generated by > utils/genrandconfig in the Buildroot tree. Besides the obvious package > randomization, there is some randomization of "global" options: > # Per-package folder > if randint(0, 15) == 0: > configlines.append("BR2_PER_PACKAGE_DIRECTORIES=y\n") > # Amend the configuration with a few things. > if randint(0, 20) == 0: > configlines.append("BR2_ENABLE_DEBUG=y\n") > if randint(0, 20) == 0: > configlines.append("BR2_ENABLE_RUNTIME_DEBUG=y\n") > if randint(0, 1) == 0: > configlines.append("BR2_INIT_BUSYBOX=y\n") > elif randint(0, 15) == 0: > configlines.append("BR2_INIT_SYSTEMD=y\n") > elif randint(0, 10) == 0: > configlines.append("BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y\n") > if randint(0, 20) == 0: > configlines.append("BR2_STATIC_LIBS=y\n") > if randint(0, 20) == 0: > configlines.append("BR2_PACKAGE_PYTHON_PY_ONLY=y\n") > if randint(0, 5) == 0: > configlines.append("BR2_OPTIMIZE_2=y\n") > if randint(0, 4) == 0: > configlines.append("BR2_SYSTEM_ENABLE_NLS=y\n") > if randint(0, 4) == 0: > configlines.append("BR2_FORTIFY_SOURCE_2=y\n") Thanks for valuable info! I need to have look into https://git.busybox.net/buildroot-test/. > For example, did you test with BR2_FORTIFY_SOURCE_2=y ? > Could you point me to the autobuilder failure that you had, but was not > able to reproduce with test-pkg ? We should be able to point out the > difference. Actually, I thought I submitted 0ce30de72f ("package/nfs-utils: bump to version 2.5.4"), but I didn't thus I did not run the verifies. I now tested b2857786f1 ("package/nfs-utils: needs uuid") (which I also didn't submit thus didn't tested and all verifiers are OK. I believe that was the first build which failed due warning on printf format. And now: grep ^BR2_FORTIFY_SOURCE_2 ~/br-test-pkg/*/.config # nothing I'll try to test next time with snippet with BR2_FORTIFY_SOURCE_2=y. Anything else it's worth of enabling? E.g. BR2_STATIC_LIBS have at least some of them, also BR2_FORTIFY_SOURCE_1 (but that's not enough) > The thing is that test-pkg cannot test all possibilities, it would take > way too much time. Understand. I just didn't know about the randomizer on autobuilders. I did noticed that sometimes error was reported even I was not able to spot it during test-pkg testing :). Kind regards, Petr > Thomas
On 02/08/2021 19:21, Petr Vorel wrote: > Previous fixes from daa5459b6a caused regression that even more builds > were broken. Backporting upstream fix 383d787d ("nfsdcltrack: Use > uint64_t instead of time_t") which fixes it. > It fixes bug: > nfsdcltrack.c: In function ‘cltrack_gracedone’: > nfsdcltrack.c:529:18: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘time_t’ {aka ‘long int’} [-Werror=format=] > 529 | xlog(D_GENERAL, "%s: grace done. gracetime=%"PRIu64, __func__, gracetime); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~ > | | > | time_t {aka long int} > > Fixes: > - http://autobuild.buildroot.net/results/550ad9c74666775858887050ea83a8618797de79 > - http://autobuild.buildroot.net/results/caf0a69121d49504db63910ce1df347802065cd9 > - http://autobuild.buildroot.net/results/294d11f8c4330bcfd9412f47f21df22d4f2b6e82 > - http://autobuild.buildroot.net/results/1cb6b19cbbc2b829a038a9a70b6734c084e81c93 > - http://autobuild.buildroot.net/results/c0eb120f5d14798b4f88b90eaccd3bc1f657c00d > - http://autobuild.buildroot.net/results/a028299b612a0ca6d1a69757749a8b9e37094954 > - http://autobuild.buildroot.net/results/996bb74445cfbde0e230b2185207d91d4ec46dfb > - http://autobuild.buildroot.net/results/624b6dc0793d0263f46831ed0f29fa265ff0e41f > - http://autobuild.buildroot.net/results/5eb2dd5d6bc0540443a52f0b9b9568af01892a23 > - http://autobuild.buildroot.net/results/65bad834738d5c5b1176e8847777b3cfd95c4351 > - http://autobuild.buildroot.net/results/3146237c356e5649163784fb6628fbbbc7c61153 > - http://autobuild.buildroot.net/results/2c5e77600cbf5009e69f057e7a2dc164fc8b0466 > - http://autobuild.buildroot.net/results/ec8ec029dc0d7d7d294f8ade81ee53ca2fc3c54c > - http://autobuild.buildroot.net/results/60ea3bdeddf71270468d6177ccf95a1dc6b68dac > - http://autobuild.buildroot.net/results/ff93a63cd084043db5f61ac5ea2bbbca4f0e6859 > - http://autobuild.buildroot.net/results/cac3392373d05d78c299d07d8080ae4938773401 > - http://autobuild.buildroot.net/results/438155f831d8a44ec6e0f7020e8783b48d2c8df2 > - http://autobuild.buildroot.net/results/66bc1ca1a05e1676c4e65ef7364ba1415bdbc723 > - http://autobuild.buildroot.net/results/5bacb3c38dc1012f9445a0c23e59336dcac5abce > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> Applied to master, thanks. Regards, Arnout > --- > I'm sorry for not catching these. Our testing tools didn't caught that > (probably was not using -Werror). > > Tested: > bootlin-armv5-uclibc [1/6]: OK > bootlin-armv7-glibc [2/6]: OK > bootlin-armv7m-uclibc [3/6]: SKIPPED > bootlin-x86-64-musl [4/6]: OK > br-arm-full-static [5/6]: OK > sourcery-arm [6/6]: OK > 6 builds, 1 skipped, 0 build failed, 0 legal-info failed > > andes-nds32 [ 1/45]: OK > arm-aarch64 [ 2/45]: OK > bootlin-aarch64-glibc [ 3/45]: OK > bootlin-arcle-hs38-uclibc [ 4/45]: OK > bootlin-armv5-uclibc [ 5/45]: OK > bootlin-armv7-glibc [ 6/45]: OK > bootlin-armv7m-uclibc [ 7/45]: SKIPPED > bootlin-armv7-musl [ 8/45]: OK > bootlin-microblazeel-uclibc [ 9/45]: OK > bootlin-mipsel-uclibc [10/45]: OK > bootlin-mipsel32r6-glibc [11/45]: OK > bootlin-m68k-5208-uclibc [12/45]: SKIPPED > bootlin-m68k-68040-uclibc [13/45]: OK > bootlin-nios2-glibc [14/45]: OK > bootlin-openrisc-uclibc [15/45]: OK > bootlin-powerpc-e500mc-uclibc [16/45]: OK > bootlin-powerpc64le-power8-glibc [17/45]: OK > bootlin-riscv32-glibc [18/45]: OK > bootlin-riscv64-glibc [19/45]: OK > bootlin-riscv64-musl [20/45]: OK > bootlin-sh4-uclibc [21/45]: OK > bootlin-sparc-uclibc [22/45]: OK > bootlin-sparc64-glibc [23/45]: OK > bootlin-xtensa-uclibc [24/45]: OK > bootlin-x86-64-glibc [25/45]: OK > bootlin-x86-64-musl [26/45]: OK > bootlin-x86-64-uclibc [27/45]: OK > br-arm-basic [28/45]: OK > br-arm-full-nothread [29/45]: SKIPPED > br-arm-full-static [30/45]: OK > br-i386-pentium-mmx-musl [31/45]: OK > br-i386-pentium4-full [32/45]: OK > br-mips64-n64-full [33/45]: OK > br-mips64r6-el-hf-glibc [34/45]: OK > br-powerpc-603e-basic-cpp [35/45]: OK > br-powerpc64-power7-glibc [36/45]: OK > > Kind regards, > Petr > > ...track-Use-uint64_t-instead-of-time_t.patch | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch > > diff --git a/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch b/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch > new file mode 100644 > index 0000000000..384f4fd806 > --- /dev/null > +++ b/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch > @@ -0,0 +1,66 @@ > +From 383d787d1b77f165da68495cb0363220b66935a4 Mon Sep 17 00:00:00 2001 > +From: Steve Dickson <steved@redhat.com> > +Date: Tue, 27 Jul 2021 21:12:17 -0400 > +Subject: [PATCH] nfsdcltrack: Use uint64_t instead of time_t > + > +With recent commits (4f2a5b64,5a53426c) that fixed > +compile errors on x86_64 machines, caused similar > +errors on i686 machines. > + > +The variable type that was being used was a time_t, > +which changes size between architects, which > +caused the compile error. > + > +Changing the variable to uint64_t fixed the issue. > + > +Signed-off-by: Steve Dickson <steved@redhat.com> > +Upstream: http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=commit;h=383d787d1b77f165da68495cb0363220b66935a4 > +Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > +--- > + utils/nfsdcltrack/nfsdcltrack.c | 2 +- > + utils/nfsdcltrack/sqlite.c | 2 +- > + utils/nfsdcltrack/sqlite.h | 2 +- > + 3 files changed, 3 insertions(+), 3 deletions(-) > + > +diff --git a/utils/nfsdcltrack/nfsdcltrack.c b/utils/nfsdcltrack/nfsdcltrack.c > +index 0b37c094..7c1c4bcc 100644 > +--- a/utils/nfsdcltrack/nfsdcltrack.c > ++++ b/utils/nfsdcltrack/nfsdcltrack.c > +@@ -508,7 +508,7 @@ cltrack_gracedone(const char *timestr) > + { > + int ret; > + char *tail; > +- time_t gracetime; > ++ uint64_t gracetime; > + > + > + ret = sqlite_prepare_dbh(storagedir); > +diff --git a/utils/nfsdcltrack/sqlite.c b/utils/nfsdcltrack/sqlite.c > +index cea4a411..cf0c6a45 100644 > +--- a/utils/nfsdcltrack/sqlite.c > ++++ b/utils/nfsdcltrack/sqlite.c > +@@ -540,7 +540,7 @@ out_err: > + * remove any client records that were not reclaimed since grace_start. > + */ > + int > +-sqlite_remove_unreclaimed(time_t grace_start) > ++sqlite_remove_unreclaimed(uint64_t grace_start) > + { > + int ret; > + char *err = NULL; > +diff --git a/utils/nfsdcltrack/sqlite.h b/utils/nfsdcltrack/sqlite.h > +index 06e7c044..ba8cdfa8 100644 > +--- a/utils/nfsdcltrack/sqlite.h > ++++ b/utils/nfsdcltrack/sqlite.h > +@@ -26,7 +26,7 @@ int sqlite_insert_client(const unsigned char *clname, const size_t namelen, > + int sqlite_remove_client(const unsigned char *clname, const size_t namelen); > + int sqlite_check_client(const unsigned char *clname, const size_t namelen, > + const bool has_session); > +-int sqlite_remove_unreclaimed(const time_t grace_start); > ++int sqlite_remove_unreclaimed(const uint64_t grace_start); > + int sqlite_query_reclaiming(const time_t grace_start); > + > + #endif /* _SQLITE_H */ > +-- > +2.32.0 > + >
On 03/08/2021 19:17, Petr Vorel wrote: > Hi Thomas, > >> On Tue, 3 Aug 2021 00:06:46 +0200 >> Petr Vorel <petr.vorel@gmail.com> wrote: > >>> I believe all 3 patches (2 already merged + this one) should fix the problem. >>> But I was not able to verify it, because ./utils/test-pkg didn't catch even >>> this error (I tested all available toolchains), IMHO -Werror=format=2 and other >>> -Werror are probably only on http://autobuild.buildroot.net/ (not in Buildroot >>> config for users). It'd be great if ./utils/test-pkg had the same CFLAGS. >>> Or have I (again) overlooked something? > >> No, the autobuilders don't do anything specific with -Werror CFLAGS. >> The configurations tested by the autobuilders are generated by >> utils/genrandconfig in the Buildroot tree. Besides the obvious package >> randomization, there is some randomization of "global" options: > >> # Per-package folder >> if randint(0, 15) == 0: >> configlines.append("BR2_PER_PACKAGE_DIRECTORIES=y\n") > >> # Amend the configuration with a few things. >> if randint(0, 20) == 0: >> configlines.append("BR2_ENABLE_DEBUG=y\n") >> if randint(0, 20) == 0: >> configlines.append("BR2_ENABLE_RUNTIME_DEBUG=y\n") >> if randint(0, 1) == 0: >> configlines.append("BR2_INIT_BUSYBOX=y\n") >> elif randint(0, 15) == 0: >> configlines.append("BR2_INIT_SYSTEMD=y\n") >> elif randint(0, 10) == 0: >> configlines.append("BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y\n") >> if randint(0, 20) == 0: >> configlines.append("BR2_STATIC_LIBS=y\n") >> if randint(0, 20) == 0: >> configlines.append("BR2_PACKAGE_PYTHON_PY_ONLY=y\n") >> if randint(0, 5) == 0: >> configlines.append("BR2_OPTIMIZE_2=y\n") >> if randint(0, 4) == 0: >> configlines.append("BR2_SYSTEM_ENABLE_NLS=y\n") >> if randint(0, 4) == 0: >> configlines.append("BR2_FORTIFY_SOURCE_2=y\n") > Thanks for valuable info! > I need to have look into https://git.busybox.net/buildroot-test/. The genrandconfig script is in utils/ in the buildroot tree itself. The buildroot-test script just calls genrandconfig, followed by make and make legal-info. The only interesting thing for you in there is how reproducible builds are handled. > >> For example, did you test with BR2_FORTIFY_SOURCE_2=y ? > >> Could you point me to the autobuilder failure that you had, but was not >> able to reproduce with test-pkg ? We should be able to point out the >> difference. > Actually, I thought I submitted 0ce30de72f ("package/nfs-utils: bump to version > 2.5.4"), but I didn't thus I did not run the verifies. > > I now tested b2857786f1 ("package/nfs-utils: needs uuid") (which I also didn't The recent issues happen on basically any config and were introduced by daa5459b6aaa which fixed it for riscv and broke it for all other 32-bit platforms. Only, the two files concerned are only built if sqlite is enable resp. if NFSv4 is enabled, so if you don't add those to your test config, test-pkg won't catch it. Regards, Arnout > submit thus didn't tested and all verifiers are OK. I believe that was the first > build which failed due warning on printf format. And now: > > grep ^BR2_FORTIFY_SOURCE_2 ~/br-test-pkg/*/.config > # nothing > > I'll try to test next time with snippet with BR2_FORTIFY_SOURCE_2=y. > Anything else it's worth of enabling? E.g. BR2_STATIC_LIBS have at least some of > them, also BR2_FORTIFY_SOURCE_1 (but that's not enough) > >> The thing is that test-pkg cannot test all possibilities, it would take >> way too much time. > Understand. I just didn't know about the randomizer on autobuilders. I did > noticed that sometimes error was reported even I was not able to spot it during > test-pkg testing :). > > Kind regards, > Petr > >> Thomas > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
> On 03/08/2021 19:17, Petr Vorel wrote: > > Hi Thomas, > >> On Tue, 3 Aug 2021 00:06:46 +0200 > >> Petr Vorel <petr.vorel@gmail.com> wrote: > >>> I believe all 3 patches (2 already merged + this one) should fix the problem. > >>> But I was not able to verify it, because ./utils/test-pkg didn't catch even > >>> this error (I tested all available toolchains), IMHO -Werror=format=2 and other > >>> -Werror are probably only on http://autobuild.buildroot.net/ (not in Buildroot > >>> config for users). It'd be great if ./utils/test-pkg had the same CFLAGS. > >>> Or have I (again) overlooked something? > >> No, the autobuilders don't do anything specific with -Werror CFLAGS. > >> The configurations tested by the autobuilders are generated by > >> utils/genrandconfig in the Buildroot tree. Besides the obvious package > >> randomization, there is some randomization of "global" options: > >> # Per-package folder > >> if randint(0, 15) == 0: > >> configlines.append("BR2_PER_PACKAGE_DIRECTORIES=y\n") > >> # Amend the configuration with a few things. > >> if randint(0, 20) == 0: > >> configlines.append("BR2_ENABLE_DEBUG=y\n") > >> if randint(0, 20) == 0: > >> configlines.append("BR2_ENABLE_RUNTIME_DEBUG=y\n") > >> if randint(0, 1) == 0: > >> configlines.append("BR2_INIT_BUSYBOX=y\n") > >> elif randint(0, 15) == 0: > >> configlines.append("BR2_INIT_SYSTEMD=y\n") > >> elif randint(0, 10) == 0: > >> configlines.append("BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y\n") > >> if randint(0, 20) == 0: > >> configlines.append("BR2_STATIC_LIBS=y\n") > >> if randint(0, 20) == 0: > >> configlines.append("BR2_PACKAGE_PYTHON_PY_ONLY=y\n") > >> if randint(0, 5) == 0: > >> configlines.append("BR2_OPTIMIZE_2=y\n") > >> if randint(0, 4) == 0: > >> configlines.append("BR2_SYSTEM_ENABLE_NLS=y\n") > >> if randint(0, 4) == 0: > >> configlines.append("BR2_FORTIFY_SOURCE_2=y\n") > > Thanks for valuable info! > > I need to have look into https://git.busybox.net/buildroot-test/. > The genrandconfig script is in utils/ in the buildroot tree itself. The > buildroot-test script just calls genrandconfig, followed by make and make > legal-info. The only interesting thing for you in there is how reproducible > builds are handled. Thanks! > >> For example, did you test with BR2_FORTIFY_SOURCE_2=y ? > >> Could you point me to the autobuilder failure that you had, but was not > >> able to reproduce with test-pkg ? We should be able to point out the > >> difference. > > Actually, I thought I submitted 0ce30de72f ("package/nfs-utils: bump to version > > 2.5.4"), but I didn't thus I did not run the verifies. > > I now tested b2857786f1 ("package/nfs-utils: needs uuid") (which I also didn't > The recent issues happen on basically any config and were introduced by > daa5459b6aaa which fixed it for riscv and broke it for all other 32-bit > platforms. Only, the two files concerned are only built if sqlite is enable > resp. if NFSv4 is enabled, so if you don't add those to your test config, > test-pkg won't catch it. Yes, the problem was that I didn't realize that running test-pkg with default options isn't enough. Because that I validate - my patches for fixing RISC (problem which I found via autobuild), but I didn't check what needs to be enabled to actually trigger the bug. I usually found arch/toolchain specific problems with autobuild. But I see I need to do more investigation before running test-pkg. Lesson learned, thanks! Kind regards, Petr > Regards, > Arnout > > submit thus didn't tested and all verifiers are OK. I believe that was the first > > build which failed due warning on printf format. And now: > > grep ^BR2_FORTIFY_SOURCE_2 ~/br-test-pkg/*/.config > > # nothing > > I'll try to test next time with snippet with BR2_FORTIFY_SOURCE_2=y. > > Anything else it's worth of enabling? E.g. BR2_STATIC_LIBS have at least some of > > them, also BR2_FORTIFY_SOURCE_1 (but that's not enough) > >> The thing is that test-pkg cannot test all possibilities, it would take > >> way too much time. > > Understand. I just didn't know about the randomizer on autobuilders. I did > > noticed that sometimes error was reported even I was not able to spot it during > > test-pkg testing :). > > Kind regards, > > Petr > >> Thomas > > _______________________________________________ > > buildroot mailing list > > buildroot@busybox.net > > http://lists.busybox.net/mailman/listinfo/buildroot
diff --git a/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch b/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch new file mode 100644 index 0000000000..384f4fd806 --- /dev/null +++ b/package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch @@ -0,0 +1,66 @@ +From 383d787d1b77f165da68495cb0363220b66935a4 Mon Sep 17 00:00:00 2001 +From: Steve Dickson <steved@redhat.com> +Date: Tue, 27 Jul 2021 21:12:17 -0400 +Subject: [PATCH] nfsdcltrack: Use uint64_t instead of time_t + +With recent commits (4f2a5b64,5a53426c) that fixed +compile errors on x86_64 machines, caused similar +errors on i686 machines. + +The variable type that was being used was a time_t, +which changes size between architects, which +caused the compile error. + +Changing the variable to uint64_t fixed the issue. + +Signed-off-by: Steve Dickson <steved@redhat.com> +Upstream: http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=commit;h=383d787d1b77f165da68495cb0363220b66935a4 +Signed-off-by: Petr Vorel <petr.vorel@gmail.com> +--- + utils/nfsdcltrack/nfsdcltrack.c | 2 +- + utils/nfsdcltrack/sqlite.c | 2 +- + utils/nfsdcltrack/sqlite.h | 2 +- + 3 files changed, 3 insertions(+), 3 deletions(-) + +diff --git a/utils/nfsdcltrack/nfsdcltrack.c b/utils/nfsdcltrack/nfsdcltrack.c +index 0b37c094..7c1c4bcc 100644 +--- a/utils/nfsdcltrack/nfsdcltrack.c ++++ b/utils/nfsdcltrack/nfsdcltrack.c +@@ -508,7 +508,7 @@ cltrack_gracedone(const char *timestr) + { + int ret; + char *tail; +- time_t gracetime; ++ uint64_t gracetime; + + + ret = sqlite_prepare_dbh(storagedir); +diff --git a/utils/nfsdcltrack/sqlite.c b/utils/nfsdcltrack/sqlite.c +index cea4a411..cf0c6a45 100644 +--- a/utils/nfsdcltrack/sqlite.c ++++ b/utils/nfsdcltrack/sqlite.c +@@ -540,7 +540,7 @@ out_err: + * remove any client records that were not reclaimed since grace_start. + */ + int +-sqlite_remove_unreclaimed(time_t grace_start) ++sqlite_remove_unreclaimed(uint64_t grace_start) + { + int ret; + char *err = NULL; +diff --git a/utils/nfsdcltrack/sqlite.h b/utils/nfsdcltrack/sqlite.h +index 06e7c044..ba8cdfa8 100644 +--- a/utils/nfsdcltrack/sqlite.h ++++ b/utils/nfsdcltrack/sqlite.h +@@ -26,7 +26,7 @@ int sqlite_insert_client(const unsigned char *clname, const size_t namelen, + int sqlite_remove_client(const unsigned char *clname, const size_t namelen); + int sqlite_check_client(const unsigned char *clname, const size_t namelen, + const bool has_session); +-int sqlite_remove_unreclaimed(const time_t grace_start); ++int sqlite_remove_unreclaimed(const uint64_t grace_start); + int sqlite_query_reclaiming(const time_t grace_start); + + #endif /* _SQLITE_H */ +-- +2.32.0 +
Previous fixes from daa5459b6a caused regression that even more builds were broken. Backporting upstream fix 383d787d ("nfsdcltrack: Use uint64_t instead of time_t") which fixes it. It fixes bug: nfsdcltrack.c: In function ‘cltrack_gracedone’: nfsdcltrack.c:529:18: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘time_t’ {aka ‘long int’} [-Werror=format=] 529 | xlog(D_GENERAL, "%s: grace done. gracetime=%"PRIu64, __func__, gracetime); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~ | | | time_t {aka long int} Fixes: - http://autobuild.buildroot.net/results/550ad9c74666775858887050ea83a8618797de79 - http://autobuild.buildroot.net/results/caf0a69121d49504db63910ce1df347802065cd9 - http://autobuild.buildroot.net/results/294d11f8c4330bcfd9412f47f21df22d4f2b6e82 - http://autobuild.buildroot.net/results/1cb6b19cbbc2b829a038a9a70b6734c084e81c93 - http://autobuild.buildroot.net/results/c0eb120f5d14798b4f88b90eaccd3bc1f657c00d - http://autobuild.buildroot.net/results/a028299b612a0ca6d1a69757749a8b9e37094954 - http://autobuild.buildroot.net/results/996bb74445cfbde0e230b2185207d91d4ec46dfb - http://autobuild.buildroot.net/results/624b6dc0793d0263f46831ed0f29fa265ff0e41f - http://autobuild.buildroot.net/results/5eb2dd5d6bc0540443a52f0b9b9568af01892a23 - http://autobuild.buildroot.net/results/65bad834738d5c5b1176e8847777b3cfd95c4351 - http://autobuild.buildroot.net/results/3146237c356e5649163784fb6628fbbbc7c61153 - http://autobuild.buildroot.net/results/2c5e77600cbf5009e69f057e7a2dc164fc8b0466 - http://autobuild.buildroot.net/results/ec8ec029dc0d7d7d294f8ade81ee53ca2fc3c54c - http://autobuild.buildroot.net/results/60ea3bdeddf71270468d6177ccf95a1dc6b68dac - http://autobuild.buildroot.net/results/ff93a63cd084043db5f61ac5ea2bbbca4f0e6859 - http://autobuild.buildroot.net/results/cac3392373d05d78c299d07d8080ae4938773401 - http://autobuild.buildroot.net/results/438155f831d8a44ec6e0f7020e8783b48d2c8df2 - http://autobuild.buildroot.net/results/66bc1ca1a05e1676c4e65ef7364ba1415bdbc723 - http://autobuild.buildroot.net/results/5bacb3c38dc1012f9445a0c23e59336dcac5abce Signed-off-by: Petr Vorel <petr.vorel@gmail.com> --- I'm sorry for not catching these. Our testing tools didn't caught that (probably was not using -Werror). Tested: bootlin-armv5-uclibc [1/6]: OK bootlin-armv7-glibc [2/6]: OK bootlin-armv7m-uclibc [3/6]: SKIPPED bootlin-x86-64-musl [4/6]: OK br-arm-full-static [5/6]: OK sourcery-arm [6/6]: OK 6 builds, 1 skipped, 0 build failed, 0 legal-info failed andes-nds32 [ 1/45]: OK arm-aarch64 [ 2/45]: OK bootlin-aarch64-glibc [ 3/45]: OK bootlin-arcle-hs38-uclibc [ 4/45]: OK bootlin-armv5-uclibc [ 5/45]: OK bootlin-armv7-glibc [ 6/45]: OK bootlin-armv7m-uclibc [ 7/45]: SKIPPED bootlin-armv7-musl [ 8/45]: OK bootlin-microblazeel-uclibc [ 9/45]: OK bootlin-mipsel-uclibc [10/45]: OK bootlin-mipsel32r6-glibc [11/45]: OK bootlin-m68k-5208-uclibc [12/45]: SKIPPED bootlin-m68k-68040-uclibc [13/45]: OK bootlin-nios2-glibc [14/45]: OK bootlin-openrisc-uclibc [15/45]: OK bootlin-powerpc-e500mc-uclibc [16/45]: OK bootlin-powerpc64le-power8-glibc [17/45]: OK bootlin-riscv32-glibc [18/45]: OK bootlin-riscv64-glibc [19/45]: OK bootlin-riscv64-musl [20/45]: OK bootlin-sh4-uclibc [21/45]: OK bootlin-sparc-uclibc [22/45]: OK bootlin-sparc64-glibc [23/45]: OK bootlin-xtensa-uclibc [24/45]: OK bootlin-x86-64-glibc [25/45]: OK bootlin-x86-64-musl [26/45]: OK bootlin-x86-64-uclibc [27/45]: OK br-arm-basic [28/45]: OK br-arm-full-nothread [29/45]: SKIPPED br-arm-full-static [30/45]: OK br-i386-pentium-mmx-musl [31/45]: OK br-i386-pentium4-full [32/45]: OK br-mips64-n64-full [33/45]: OK br-mips64r6-el-hf-glibc [34/45]: OK br-powerpc-603e-basic-cpp [35/45]: OK br-powerpc64-power7-glibc [36/45]: OK Kind regards, Petr ...track-Use-uint64_t-instead-of-time_t.patch | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 package/nfs-utils/0003-nfsdcltrack-Use-uint64_t-instead-of-time_t.patch