[libcamera-devel] [PATCH v3 07/17] libcamera: base: file_descriptor: Return UniqueFD from dup()

[Jacopo Mondi]

Hi Laurent

On Mon, Nov 29, 2021 at 01:57:42AM +0200, Laurent Pinchart wrote:
> The dup() function returns a duplicate of the file descriptor. Wrapping
> it in a FileDescriptor isn't wrong as such, but it prevents from using
> it in contexts where a UniqueFD is needed. As the duplicate is
> guaranteed to have a single owner when created, return it as a UniqueFD
> instead. A FileDescriptor can easily be created from the UniqueFD if
> desired.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/base/file_descriptor.h |  2 +-
>  src/libcamera/base/file_descriptor.cpp   | 22 ++++++++++++++--------
>  2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
> index 74292eba04f5..12a43f95d414 100644
> --- a/include/libcamera/base/file_descriptor.h
> +++ b/include/libcamera/base/file_descriptor.h
> @@ -28,7 +28,7 @@ public:
>
>  	bool isValid() const { return fd_ != nullptr; }
>  	int fd() const { return fd_ ? fd_->fd() : -1; }
> -	FileDescriptor dup() const;
> +	UniqueFD dup() const;
>
>  private:
>  	class Descriptor
> diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
> index da696b2501cd..a83bf52c31e6 100644
> --- a/src/libcamera/base/file_descriptor.cpp
> +++ b/src/libcamera/base/file_descriptor.cpp
> @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other)
>   * \brief Duplicate a FileDescriptor
>   *
>   * Duplicating a FileDescriptor creates a duplicate of the wrapped file
> - * descriptor and returns a new FileDescriptor instance that wraps the
> - * duplicate. The fd() function of the original and duplicate instances will
> - * return different values. The duplicate instance will not be affected by
> - * destruction of the original instance or its copies.
> + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function
> + * of the original and the get() function of the duplicate will return different

Just noticed that we now have:
        SharedFD::fd()
        UniqueFD::get()

Should the two be named the same ?

> + * values. The duplicate instance will not be affected by destruction of the
> + * original instance or its copies.
>   *
> - * \return A new FileDescriptor instance wrapping a duplicate of the original
> - * file descriptor
> + * \return A UniqueFD owning a duplicate of the original file descriptor
>   */
> -FileDescriptor FileDescriptor::dup() const
> +UniqueFD FileDescriptor::dup() const
>  {
> -	return FileDescriptor(fd());
> +	int dupFd = ::dup(fd());
> +	if (dupFd == -1) {
> +		int ret = -errno;

As we don't need to return ret this can simply be
                in ret = errno;

> +		LOG(FileDescriptor, Error)
> +			<< "Failed to dup() fd: " << strerror(-ret);

and
                                                  << strerror(ret);

> +	}
> +
> +	return UniqueFD(dupFd);

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

>  }
>
>  FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
> --
> Regards,
>
> Laurent Pinchart
>