[PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into function

[Kieran Bingham]

Quoting Barnabás Pőcze (2024-06-18 02:06:55)
> This commit moves the check that determines whether the
> mode argument of `open*()` exists into a separate function.
> 
> With that, the check is fixed because previously it failed to
> account for the fact that `O_TMPFILE` is not a power of two.
> 
> Furthermore, add `asserts()` in the fortified variants that
> ensure that no mode is required by the specified flags.
> 
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
>  src/v4l2/v4l2_compat.cpp | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp
> index 8a44403e..5c6f925e 100644
> --- a/src/v4l2/v4l2_compat.cpp
> +++ b/src/v4l2/v4l2_compat.cpp
> @@ -7,6 +7,7 @@
>  
>  #include "v4l2_compat_manager.h"
>  
> +#include <assert.h>
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <stdarg.h>
> @@ -28,12 +29,17 @@ using namespace libcamera;
>         va_end(ap);                     \
>  }
>  

I'd be tempted to add a comment here explaining /what/ this check is
for. It wasn't obvious to me straight away.


/*
 * Determine if the flags set will require a further mode flag to be
 * parsed as an additional argument in the va_args.
 */

> +static inline bool needs_mode(int flags)
> +{
> +       return (flags & O_CREAT) || ((flags & O_TMPFILE) == O_TMPFILE);
> +}
> +
>  extern "C" {
>  
>  LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
>  {
>         mode_t mode = 0;
> -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> +       if (needs_mode(oflag))
>                 extract_va_arg(mode_t, mode, oflag);
>  
>         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,
> @@ -43,6 +49,7 @@ LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
>  /* _FORTIFY_SOURCE redirects open to __open_2 */
>  LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)
>  {
> +       assert(!needs_mode(oflag));

I was going to ask if we should be returning errors here instead, but
now I checked and I see we're in _FORTIFY_SOURCE where assertions are
perfectly valid and expected even, so I think this is fine.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>         return open(path, oflag);
>  }
>  
> @@ -50,7 +57,7 @@ LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)
>  LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)
>  {
>         mode_t mode = 0;
> -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> +       if (needs_mode(oflag))
>                 extract_va_arg(mode_t, mode, oflag);
>  
>         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,
> @@ -59,6 +66,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)
>  
>  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)
>  {
> +       assert(!needs_mode(oflag));
>         return open64(path, oflag);
>  }
>  #endif
> @@ -66,7 +74,7 @@ LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)
>  LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
>  {
>         mode_t mode = 0;
> -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> +       if (needs_mode(oflag))
>                 extract_va_arg(mode_t, mode, oflag);
>  
>         return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);
> @@ -74,6 +82,7 @@ LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
>  
>  LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)
>  {
> +       assert(!needs_mode(oflag));
>         return openat(dirfd, path, oflag);
>  }
>  
> @@ -81,7 +90,7 @@ LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)
>  LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)
>  {
>         mode_t mode = 0;
> -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> +       if (needs_mode(oflag))
>                 extract_va_arg(mode_t, mode, oflag);
>  
>         return V4L2CompatManager::instance()->openat(dirfd, path,
> @@ -90,6 +99,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)
>  
>  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)
>  {
> +       assert(!needs_mode(oflag));
>         return openat64(dirfd, path, oflag);
>  }
>  #endif
> -- 
> 2.45.2
> 
>