| Message ID | 20250725171608.1610639-1-giacomo.cappellini.87@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Giacomo, Thank you for the patch. On Fri, Jul 25, 2025 at 07:14:39PM +0200, Giacomo Cappellini wrote: > Fix dlsym lookup for 'openat64' and 'mmap64' to fallback to 'openat' and > 'mmap' on musl, since musl does not provide the LFS64 variants and this > ensures compatibility. > > musl does not support the LFS64 API, and relocs-processing is relevant > only to glibc ABI compatibility. > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com> > --- > src/v4l2/v4l2_compat_manager.cpp | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index f53fb300..b970d86c 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -43,10 +43,14 @@ V4L2CompatManager::V4L2CompatManager() > : cm_(nullptr) > { > get_symbol(fops_.openat, "openat64"); > + if (!fops_.openat) > + get_symbol(fops_.openat, "openat"); I'm slightly worried that a silent fallback may cause difficult to debug issues if for some reason the openat64 lookup fails on a non-musl libc. I can't really provide a good example though, but how about the following implementation that detects the available symbols at build time and looks up the appropriate ones ? diff --git a/meson.build b/meson.build index 4ed8017eba1a..66222d8a9d77 100644 --- a/meson.build +++ b/meson.build @@ -90,6 +90,10 @@ if cc.has_header_symbol('sys/mman.h', 'memfd_create', prefix : '#define _GNU_SOU config_h.set('HAVE_MEMFD_CREATE', 1) endif +if cc.has_function('openat64') + config_h.set('HAVE_OPENAT64', 1) +endif + ioctl_posix_test = ''' #include <sys/ioctl.h> int ioctl (int, int, ...); diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index f53fb300dde8..01b4b1460967 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -42,11 +42,16 @@ void get_symbol(T &func, const char *name) V4L2CompatManager::V4L2CompatManager() : cm_(nullptr) { +#if HAVE_OPENAT64 get_symbol(fops_.openat, "openat64"); + get_symbol(fops_.mmap, "mmap64"); +#else + get_symbol(fops_.openat, "openat"); + get_symbol(fops_.mmap, "mmap"); +#endif get_symbol(fops_.dup, "dup"); get_symbol(fops_.close, "close"); get_symbol(fops_.ioctl, "ioctl"); - get_symbol(fops_.mmap, "mmap64"); get_symbol(fops_.munmap, "munmap"); } Could you test this with musl ? > get_symbol(fops_.dup, "dup"); > get_symbol(fops_.close, "close"); > get_symbol(fops_.ioctl, "ioctl"); > get_symbol(fops_.mmap, "mmap64"); > + if (!fops_.mmap) > + get_symbol(fops_.mmap, "mmap"); > get_symbol(fops_.munmap, "munmap"); > } >
Hi Laurent, your proposed compile time solution does work on musl. If you consider the runtime solution harder to debug, let's continue with compile time solution using your proposed changes. What follow is just my opinionated take on the runtime solution, just skip if not interested (I wrote this as a note before sending the patch). 2 reasons to prefer runtime instead of the compile time solution: - LD_PRELOAD and dlsym based nature of V4L2CompatManager - a couple of null checks is an easier solution without risking anything subtly or seriously wrong As *64 bit symbols are only relevant for glibc 32 bit targets when _FILE_OFFSET_BITS is set, this handles non-standard *64 symbols, which are often transparently remapped by _FILE_OFFSET_BITS=64. The explicit runtime null checks prevent subtle issues, ensuring an easier solution for greater compatibility across diverse libc implementations. Best Regards, G.C. Il giorno sab 26 lug 2025 alle ore 00:16 Laurent Pinchart <laurent.pinchart@ideasonboard.com> ha scritto: > > Hi Giacomo, > > Thank you for the patch. > > On Fri, Jul 25, 2025 at 07:14:39PM +0200, Giacomo Cappellini wrote: > > Fix dlsym lookup for 'openat64' and 'mmap64' to fallback to 'openat' and > > 'mmap' on musl, since musl does not provide the LFS64 variants and this > > ensures compatibility. > > > > musl does not support the LFS64 API, and relocs-processing is relevant > > only to glibc ABI compatibility. > > > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com> > > --- > > src/v4l2/v4l2_compat_manager.cpp | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > index f53fb300..b970d86c 100644 > > --- a/src/v4l2/v4l2_compat_manager.cpp > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > @@ -43,10 +43,14 @@ V4L2CompatManager::V4L2CompatManager() > > : cm_(nullptr) > > { > > get_symbol(fops_.openat, "openat64"); > > + if (!fops_.openat) > > + get_symbol(fops_.openat, "openat"); > > I'm slightly worried that a silent fallback may cause difficult to debug > issues if for some reason the openat64 lookup fails on a non-musl libc. > I can't really provide a good example though, but how about the > following implementation that detects the available symbols at build > time and looks up the appropriate ones ? > > diff --git a/meson.build b/meson.build > index 4ed8017eba1a..66222d8a9d77 100644 > --- a/meson.build > +++ b/meson.build > @@ -90,6 +90,10 @@ if cc.has_header_symbol('sys/mman.h', 'memfd_create', prefix : '#define _GNU_SOU > config_h.set('HAVE_MEMFD_CREATE', 1) > endif > > +if cc.has_function('openat64') > + config_h.set('HAVE_OPENAT64', 1) > +endif > + > ioctl_posix_test = ''' > #include <sys/ioctl.h> > int ioctl (int, int, ...); > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index f53fb300dde8..01b4b1460967 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -42,11 +42,16 @@ void get_symbol(T &func, const char *name) > V4L2CompatManager::V4L2CompatManager() > : cm_(nullptr) > { > +#if HAVE_OPENAT64 > get_symbol(fops_.openat, "openat64"); > + get_symbol(fops_.mmap, "mmap64"); > +#else > + get_symbol(fops_.openat, "openat"); > + get_symbol(fops_.mmap, "mmap"); > +#endif > get_symbol(fops_.dup, "dup"); > get_symbol(fops_.close, "close"); > get_symbol(fops_.ioctl, "ioctl"); > - get_symbol(fops_.mmap, "mmap64"); > get_symbol(fops_.munmap, "munmap"); > } > > > Could you test this with musl ? > > > get_symbol(fops_.dup, "dup"); > > get_symbol(fops_.close, "close"); > > get_symbol(fops_.ioctl, "ioctl"); > > get_symbol(fops_.mmap, "mmap64"); > > + if (!fops_.mmap) > > + get_symbol(fops_.mmap, "mmap"); > > get_symbol(fops_.munmap, "munmap"); > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Giacomo, On Sat, Jul 26, 2025 at 02:18:21AM +0200, Giacomo Cappellini wrote: > Hi Laurent, > > your proposed compile time solution does work on musl. > If you consider the runtime solution harder to debug, let's continue > with compile time solution using your proposed changes. > > What follow is just my opinionated take on the runtime solution, just > skip if not interested (I wrote this as a note before sending the > patch). I'm always (OK, nearly always :-)) interesting in opinions different than mine. Thank you for sharing. > 2 reasons to prefer runtime instead of the compile time solution: > - LD_PRELOAD and dlsym based nature of V4L2CompatManager I'm not sure what you mean by this, how does that relate to runtime vs. build time decision ? > - a couple of null checks is an easier solution without risking > anything subtly or seriously wrong > As *64 bit symbols are only relevant for glibc 32 bit targets when > _FILE_OFFSET_BITS is set, this handles non-standard *64 symbols, which > are often transparently remapped by _FILE_OFFSET_BITS=64. I'm also not sure to follow you here. The 64 bit symbols exist on both 32-bit and 64-bit targets. On 64-bit targets the mmap function is redirected to mmap64 transparently if _FILE_OFFSET_BITS is defined to 64, but the symbol that we need is mmap64. What do you mean by non-standard *64 symbols ? > The explicit runtime null checks prevent subtle issues, ensuring an > easier solution for greater compatibility across diverse libc > implementations. My concern is that we need openat and mmap functions that match the 64-bit prototypes. On glibc (and uclibc) those are the *64 symbols, while on musl they are the non-64 symbols. If we pick a symbol for a 32-bit function which thinking it matches the 64-bit API, there will be issues difficult to debug. > Il giorno sab 26 lug 2025 alle ore 00:16 Laurent Pinchart ha scritto: > > On Fri, Jul 25, 2025 at 07:14:39PM +0200, Giacomo Cappellini wrote: > > > Fix dlsym lookup for 'openat64' and 'mmap64' to fallback to 'openat' and > > > 'mmap' on musl, since musl does not provide the LFS64 variants and this > > > ensures compatibility. > > > > > > musl does not support the LFS64 API, and relocs-processing is relevant > > > only to glibc ABI compatibility. > > > > > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com> > > > --- > > > src/v4l2/v4l2_compat_manager.cpp | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > > index f53fb300..b970d86c 100644 > > > --- a/src/v4l2/v4l2_compat_manager.cpp > > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > > @@ -43,10 +43,14 @@ V4L2CompatManager::V4L2CompatManager() > > > : cm_(nullptr) > > > { > > > get_symbol(fops_.openat, "openat64"); > > > + if (!fops_.openat) > > > + get_symbol(fops_.openat, "openat"); > > > > I'm slightly worried that a silent fallback may cause difficult to debug > > issues if for some reason the openat64 lookup fails on a non-musl libc. > > I can't really provide a good example though, but how about the > > following implementation that detects the available symbols at build > > time and looks up the appropriate ones ? > > > > diff --git a/meson.build b/meson.build > > index 4ed8017eba1a..66222d8a9d77 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -90,6 +90,10 @@ if cc.has_header_symbol('sys/mman.h', 'memfd_create', prefix : '#define _GNU_SOU > > config_h.set('HAVE_MEMFD_CREATE', 1) > > endif > > > > +if cc.has_function('openat64') > > + config_h.set('HAVE_OPENAT64', 1) > > +endif > > + > > ioctl_posix_test = ''' > > #include <sys/ioctl.h> > > int ioctl (int, int, ...); > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > index f53fb300dde8..01b4b1460967 100644 > > --- a/src/v4l2/v4l2_compat_manager.cpp > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > @@ -42,11 +42,16 @@ void get_symbol(T &func, const char *name) > > V4L2CompatManager::V4L2CompatManager() > > : cm_(nullptr) > > { > > +#if HAVE_OPENAT64 > > get_symbol(fops_.openat, "openat64"); > > + get_symbol(fops_.mmap, "mmap64"); > > +#else > > + get_symbol(fops_.openat, "openat"); > > + get_symbol(fops_.mmap, "mmap"); > > +#endif > > get_symbol(fops_.dup, "dup"); > > get_symbol(fops_.close, "close"); > > get_symbol(fops_.ioctl, "ioctl"); > > - get_symbol(fops_.mmap, "mmap64"); > > get_symbol(fops_.munmap, "munmap"); > > } > > > > > > Could you test this with musl ? > > > > > get_symbol(fops_.dup, "dup"); > > > get_symbol(fops_.close, "close"); > > > get_symbol(fops_.ioctl, "ioctl"); > > > get_symbol(fops_.mmap, "mmap64"); > > > + if (!fops_.mmap) > > > + get_symbol(fops_.mmap, "mmap"); > > > get_symbol(fops_.munmap, "munmap"); > > > } > > >
diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index f53fb300..b970d86c 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -43,10 +43,14 @@ V4L2CompatManager::V4L2CompatManager() : cm_(nullptr) { get_symbol(fops_.openat, "openat64"); + if (!fops_.openat) + get_symbol(fops_.openat, "openat"); get_symbol(fops_.dup, "dup"); get_symbol(fops_.close, "close"); get_symbol(fops_.ioctl, "ioctl"); get_symbol(fops_.mmap, "mmap64"); + if (!fops_.mmap) + get_symbol(fops_.mmap, "mmap"); get_symbol(fops_.munmap, "munmap"); }
Fix dlsym lookup for 'openat64' and 'mmap64' to fallback to 'openat' and 'mmap' on musl, since musl does not provide the LFS64 variants and this ensures compatibility. musl does not support the LFS64 API, and relocs-processing is relevant only to glibc ABI compatibility. Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com> --- src/v4l2/v4l2_compat_manager.cpp | 4 ++++ 1 file changed, 4 insertions(+)