[libcamera-devel] [PATCH v4 16/22] libcamera: v4l2_videodevice: Pass SharedFD to open()

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Tue, Nov 30, 2021 at 12:39 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> The V4L2VideoDevice::open() function that takes an open file handle
> duplicates it internally, and leaves the original handle untouched. This
> is documented but not enforced through language constructs. Fix it by
> passing a SharedFD to the function.

FileDescriptor.
ditto for the commit title.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>


> ---
>  include/libcamera/internal/v4l2_videodevice.h |  2 +-
>  src/libcamera/v4l2_videodevice.cpp            | 22 ++++++++-----------
>  2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 80b665771782..9f556f99587c 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -184,7 +184,7 @@ public:
>         ~V4L2VideoDevice();
>
>         int open();
> -       int open(int handle, enum v4l2_buf_type type);
> +       int open(FileDescriptor handle, enum v4l2_buf_type type);
>         void close();
>
>         const char *driverName() const { return caps_.driver(); }
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 3966483a365f..5f1cc6e2f47f 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -612,16 +612,14 @@ int V4L2VideoDevice::open()
>   * its type from the capabilities. This can be used to force a given device type
>   * for memory-to-memory devices.
>   *
> - * The file descriptor \a handle is duplicated, and the caller is responsible
> - * for closing the \a handle when it has no further use for it. The close()
> - * function will close the duplicated file descriptor, leaving \a handle
> - * untouched.
> + * The file descriptor \a handle is duplicated, no reference to the original
> + * handle is kept.
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> +int V4L2VideoDevice::open(FileDescriptor handle, enum v4l2_buf_type type)
>  {
> -       UniqueFD newFd(dup(handle));
> +       UniqueFD newFd = handle.dup();
>         if (!newFd.isValid()) {
>                 LOG(V4L2, Error) << "Failed to duplicate file handle: "
>                                  << strerror(errno);
> @@ -1900,12 +1898,10 @@ int V4L2M2MDevice::open()
>
>         /*
>          * The output and capture V4L2VideoDevice instances use the same file
> -        * handle for the same device node. The local file handle can be closed
> -        * as the V4L2VideoDevice::open() retains a handle by duplicating the
> -        * fd passed in.
> +        * handle for the same device node.
>          */
> -       UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),
> -                           O_RDWR | O_NONBLOCK));
> +       FileDescriptor fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),
> +                                 O_RDWR | O_NONBLOCK));
>         if (!fd.isValid()) {
>                 ret = -errno;
>                 LOG(V4L2, Error) << "Failed to open V4L2 M2M device: "
> @@ -1913,11 +1909,11 @@ int V4L2M2MDevice::open()
>                 return ret;
>         }
>
> -       ret = output_->open(fd.get(), V4L2_BUF_TYPE_VIDEO_OUTPUT);
> +       ret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>         if (ret)
>                 goto err;
>
> -       ret = capture_->open(fd.get(), V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +       ret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>         if (ret)
>                 goto err;
>
> --
> Regards,
>
> Laurent Pinchart
>