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.

+#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.


+#if defined __ELF__ && (defined __GNUC__ || defined __clang__) \
+        && defined HAVE_ASM_IFNDEF_ENDIF_BALIGN && HAVE_ASM_IFNDEF_ENDIF_BALIGN

This checks HAVE_ASM_IFNDEF_ENDIF_BALIGN twice; once should suffice. Or did you mean to check HAVE_DLOPEN instead?

+          [[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.

+License:
+LGPLv2+

This module is a little unusual in that the main source code is MIT-0 rather than LGPLv2+. Although it's not incorrect to mark it as LGPLv2+ here since MIT-0 is compatible, perhaps we should instead add support for a new license category 'MIT-0'? (Though that'd be some work.)


+#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?


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?


Reply via email to