On Sat, 18 Jul 2026 at 07:17, Paul Eggert <[email protected]> wrote: > > Thanks for looking into this. Some comments on the first patch: > > On 2026-07-17 17:11, [email protected] wrote: > > > diff --git a/lib/sd-dlopen.h b/lib/sd-dlopen.h > > new file mode 100644 > > I see that this file differs slightly from the latest commit > bb3fcd73ce13eb796fe72da4202422e105f7b711 for systemd on GitHub. I assume > that the intent is to keep the copies identical, so that what's in > Gnulib will exactly match what's in the systemd sources. To that end, > the patch should contain changes to config/srclist.txt and > config/srclistvars.sh so that updates in the systemd sources are > communicated automatically to Gnulib. I can help doing that if it's not > obvious.
It requires changes following your earlier requests, that are specific to gnulib, so it cannot be synced automatically. TBH I don't foresee many changes coming to it, so it might not be worth worrying about that? > > +#include <stdint.h> > > There seems to be no real need for sd-dlopen.h to include <stdint.h>. > The only use of stdint.h concepts I see is in "#define > SD_ELF_NOTE_DLOPEN_TYPE UINT32_C(0x407c0c0a)". But this can be rewritten > as "#define SD_ELF_NOTE_DLOPEN_TYPE 0x407c0c0aU", which has the same > meaning on all platforms that will use SD_ELF_NOTE_DLOPEN_TYPE. It would > be better for sd-dlopen.h to not include <stdint.h>, so that includers > of sd-dlopen.h have a cleaner namespace, and so that the Gnulib module > need not depend on the stdint-h module. > > Similarly, tests/test-sd-dlopen.c need not use UINT32_C. Sure (this is one more change that desyncs the copy as per above) > > + [[void *handle = dlopen (0, RTLD_LAZY); > jj... > > + [[void *handle = dlopen (0, RTLD_LAZY); > > This should use NULL rather than 0, as compilers are starting to > complain about 0 in pointer contexts. ok > > +#ifdef _SD_ELF_NOTE_DLOPEN > > +# error "sd-dlopen implementation enabled without assembler support" > > +#endif > > This is in a test program. Is _SD_ELF_NOTE_DLOPEN intended to be a > public symbol, visible to test programs? If not, perhaps it's better to > not use it in test programs, which normally should rely only on the > public interfaces. > > What are the test programs testing for? Merely that the macros compile? > Would it be better for them to also test that the ELF notes are actually > inserted into the executable? Yes ensuring it builds fine, as there's some nuance due to the .asm usage. The current version is verified to work on every architecture debian supports, which should be pretty much everything. Added tests that check that the binary is stamped correctly too. > Also, is this module intended to be used only within Gnulib, as a > modules that other Gnulib modules depend on? Or can it be used directly > by an application? For example, can coreutils use it for how GNU 'sort' > uses dlopen? The intention is for both use cases.
