| Message ID | 20240731135936.2105-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart (2024-07-31 14:59:34) > uClibc doesn't provide memfd_create(), which led libcamera to open-code > the call using syscall(). Sprinkling the code with #ifdef's isn't the > most readable option, so improve it by providing a local implementation > of memfd_create(), and call the function unconditionally from > MemFd::create(). This makes the main code path more readable. > > Suggested-by: Nicolas Dufresne <nicolas@ndufresne.ca> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/base/memfd.cpp | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp > index 72474307af09..d95adb91615c 100644 > --- a/src/libcamera/base/memfd.cpp > +++ b/src/libcamera/base/memfd.cpp > @@ -20,15 +20,26 @@ > * \brief Anonymous file creation > */ > > -/* uClibc doesn't provide the file sealing API. */ > #ifndef __DOXYGEN__ > +namespace { > + > +/* uClibc doesn't provide the file sealing API. */ > #if not HAVE_FILE_SEALS > #define F_ADD_SEALS 1033 > #define F_SEAL_SHRINK 0x0002 > #define F_SEAL_GROW 0x0004 > #endif > + > +#ifndef HAVE_MEMFD_CREATE > +int memfd_create(const char *name, unsigned int flags) > +{ > + return syscall(SYS_memfd_create, name, flags); > +} > #endif > > +} /* namespace */ > +#endif /* __DOXYGEN__ */ > + > namespace libcamera { > > LOG_DECLARE_CATEGORY(File) > @@ -72,11 +83,7 @@ LOG_DECLARE_CATEGORY(File) > */ > UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) > { > -#if HAVE_MEMFD_CREATE > int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#else > - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#endif Yes, this looks neater to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > if (ret < 0) { > ret = errno; > LOG(File, Error) > -- > Regards, > > Laurent Pinchart >
Hi, Le mercredi 31 juillet 2024 à 16:59 +0300, Laurent Pinchart a écrit : > uClibc doesn't provide memfd_create(), which led libcamera to open-code > the call using syscall(). Sprinkling the code with #ifdef's isn't the > most readable option, so improve it by providing a local implementation > of memfd_create(), and call the function unconditionally from > MemFd::create(). This makes the main code path more readable. > > Suggested-by: Nicolas Dufresne <nicolas@ndufresne.ca> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/base/memfd.cpp | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp > index 72474307af09..d95adb91615c 100644 > --- a/src/libcamera/base/memfd.cpp > +++ b/src/libcamera/base/memfd.cpp > @@ -20,15 +20,26 @@ > * \brief Anonymous file creation > */ > > -/* uClibc doesn't provide the file sealing API. */ > #ifndef __DOXYGEN__ > +namespace { > + > +/* uClibc doesn't provide the file sealing API. */ > #if not HAVE_FILE_SEALS > #define F_ADD_SEALS 1033 > #define F_SEAL_SHRINK 0x0002 > #define F_SEAL_GROW 0x0004 > #endif > + > +#ifndef HAVE_MEMFD_CREATE > +int memfd_create(const char *name, unsigned int flags) This is not being exported, so perhaps `static int ..` Or is that the default i .cpp ? > +{ > + return syscall(SYS_memfd_create, name, flags); > +} > #endif > > +} /* namespace */ > +#endif /* __DOXYGEN__ */ > + > namespace libcamera { > > LOG_DECLARE_CATEGORY(File) > @@ -72,11 +83,7 @@ LOG_DECLARE_CATEGORY(File) > */ > UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) > { > -#if HAVE_MEMFD_CREATE > int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#else > - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#endif > if (ret < 0) { > ret = errno; > LOG(File, Error) A lot nicer to me (of course I'm biased from having suggested it). Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Hi, On 7/31/24 3:59 PM, Laurent Pinchart wrote: > uClibc doesn't provide memfd_create(), which led libcamera to open-code > the call using syscall(). Sprinkling the code with #ifdef's isn't the > most readable option, so improve it by providing a local implementation > of memfd_create(), and call the function unconditionally from > MemFd::create(). This makes the main code path more readable. > > Suggested-by: Nicolas Dufresne <nicolas@ndufresne.ca> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/base/memfd.cpp | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp > index 72474307af09..d95adb91615c 100644 > --- a/src/libcamera/base/memfd.cpp > +++ b/src/libcamera/base/memfd.cpp > @@ -20,15 +20,26 @@ > * \brief Anonymous file creation > */ > > -/* uClibc doesn't provide the file sealing API. */ > #ifndef __DOXYGEN__ > +namespace { > + > +/* uClibc doesn't provide the file sealing API. */ > #if not HAVE_FILE_SEALS > #define F_ADD_SEALS 1033 > #define F_SEAL_SHRINK 0x0002 > #define F_SEAL_GROW 0x0004 > #endif > + > +#ifndef HAVE_MEMFD_CREATE Hmm below you are removing: "#if HAVE_MEMFD_CREATE" Which suggests that HAVE_MEMFD_CREATE is always defined as either 0 or 1 and above you use "#if not HAVE_FILE_SEALS" I would prefer: #if not HAVE_MEMFD_CREATE to match the "#if not HAVE_FILE_SEALS" for consistency. otherwise this looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > +int memfd_create(const char *name, unsigned int flags) > +{ > + return syscall(SYS_memfd_create, name, flags); > +} > #endif > > +} /* namespace */ > +#endif /* __DOXYGEN__ */ > + > namespace libcamera { > > LOG_DECLARE_CATEGORY(File) > @@ -72,11 +83,7 @@ LOG_DECLARE_CATEGORY(File) > */ > UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) > { > -#if HAVE_MEMFD_CREATE > int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#else > - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#endif > if (ret < 0) { > ret = errno; > LOG(File, Error)
Hi Nicolas, On Wed, Jul 31, 2024 at 02:49:25PM -0400, Nicolas Dufresne wrote: > Le mercredi 31 juillet 2024 à 16:59 +0300, Laurent Pinchart a écrit : > > uClibc doesn't provide memfd_create(), which led libcamera to open-code > > the call using syscall(). Sprinkling the code with #ifdef's isn't the > > most readable option, so improve it by providing a local implementation > > of memfd_create(), and call the function unconditionally from > > MemFd::create(). This makes the main code path more readable. > > > > Suggested-by: Nicolas Dufresne <nicolas@ndufresne.ca> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/base/memfd.cpp | 17 ++++++++++++----- > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp > > index 72474307af09..d95adb91615c 100644 > > --- a/src/libcamera/base/memfd.cpp > > +++ b/src/libcamera/base/memfd.cpp > > @@ -20,15 +20,26 @@ > > * \brief Anonymous file creation > > */ > > > > -/* uClibc doesn't provide the file sealing API. */ > > #ifndef __DOXYGEN__ > > +namespace { > > + > > +/* uClibc doesn't provide the file sealing API. */ > > #if not HAVE_FILE_SEALS > > #define F_ADD_SEALS 1033 > > #define F_SEAL_SHRINK 0x0002 > > #define F_SEAL_GROW 0x0004 > > #endif > > + > > +#ifndef HAVE_MEMFD_CREATE > > +int memfd_create(const char *name, unsigned int flags) > > This is not being exported, so perhaps `static int ..` Or is that the default i > .cpp ? The preferred way to make symbols local in C++ is to enclose them in an anonymous namespace: /* This will be visible outside of the compilation unit */ int foo() { return 42; } namespace { /* This won't */ int bar() { return 24; } } > > +{ > > + return syscall(SYS_memfd_create, name, flags); > > +} > > #endif > > > > +} /* namespace */ > > +#endif /* __DOXYGEN__ */ > > + > > namespace libcamera { > > > > LOG_DECLARE_CATEGORY(File) > > @@ -72,11 +83,7 @@ LOG_DECLARE_CATEGORY(File) > > */ > > UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) > > { > > -#if HAVE_MEMFD_CREATE > > int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > > -#else > > - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > > -#endif > > if (ret < 0) { > > ret = errno; > > LOG(File, Error) > > A lot nicer to me (of course I'm biased from having suggested it). > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp index 72474307af09..d95adb91615c 100644 --- a/src/libcamera/base/memfd.cpp +++ b/src/libcamera/base/memfd.cpp @@ -20,15 +20,26 @@ * \brief Anonymous file creation */ -/* uClibc doesn't provide the file sealing API. */ #ifndef __DOXYGEN__ +namespace { + +/* uClibc doesn't provide the file sealing API. */ #if not HAVE_FILE_SEALS #define F_ADD_SEALS 1033 #define F_SEAL_SHRINK 0x0002 #define F_SEAL_GROW 0x0004 #endif + +#ifndef HAVE_MEMFD_CREATE +int memfd_create(const char *name, unsigned int flags) +{ + return syscall(SYS_memfd_create, name, flags); +} #endif +} /* namespace */ +#endif /* __DOXYGEN__ */ + namespace libcamera { LOG_DECLARE_CATEGORY(File) @@ -72,11 +83,7 @@ LOG_DECLARE_CATEGORY(File) */ UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) { -#if HAVE_MEMFD_CREATE int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); -#else - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); -#endif if (ret < 0) { ret = errno; LOG(File, Error)
uClibc doesn't provide memfd_create(), which led libcamera to open-code the call using syscall(). Sprinkling the code with #ifdef's isn't the most readable option, so improve it by providing a local implementation of memfd_create(), and call the function unconditionally from MemFd::create(). This makes the main code path more readable. Suggested-by: Nicolas Dufresne <nicolas@ndufresne.ca> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/base/memfd.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)