[libcamera-devel,1/3] libcamera: file_descriptor: Implement move semantics for constructor

3805 diff mbox series

The FileDescriptor class, when constructed from a numerical file
descriptor, duplicates the file descriptor and takes ownership of the
copy. The caller has to close the original file descriptor manually if
needed. This is inefficient as the dup() and close() calls could be
avoided, but can also lead to resource leakage, as recently shown by
commit 353fc4c22322 ("libcamera: v4l2_videodevice: Fix dangling file
descriptor").

In an attempt to solve this problem, implement move semantics for the
FileDescriptor constructor. The constructor taking a numerical file
descriptor is split in two variants:

- A "fd copy" constructor that takes a const lvalue reference to a
  numerical file descriptor and duplicates it (corresponding to the
  current behaviour).
- A "fd move" constructor that takes a rvalue reference to a numerical
  file descriptor and takes ownership of it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/file_descriptor.h |  5 +-
 src/libcamera/file_descriptor.cpp   | 85 ++++++++++++++++++++++-------
 2 files changed, 67 insertions(+), 23 deletions(-)

Hi Laurent,

Tanks for your work.

On 2020-05-18 19:48:02 +0300, Laurent Pinchart wrote:
> The FileDescriptor class, when constructed from a numerical file
> descriptor, duplicates the file descriptor and takes ownership of the
> copy. The caller has to close the original file descriptor manually if
> needed. This is inefficient as the dup() and close() calls could be
> avoided, but can also lead to resource leakage, as recently shown by
> commit 353fc4c22322 ("libcamera: v4l2_videodevice: Fix dangling file
> descriptor").
> 
> In an attempt to solve this problem, implement move semantics for the
> FileDescriptor constructor. The constructor taking a numerical file
> descriptor is split in two variants:
> 
> - A "fd copy" constructor that takes a const lvalue reference to a
>   numerical file descriptor and duplicates it (corresponding to the
>   current behaviour).
> - A "fd move" constructor that takes a rvalue reference to a numerical
>   file descriptor and takes ownership of it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/file_descriptor.h |  5 +-
>  src/libcamera/file_descriptor.cpp   | 85 ++++++++++++++++++++++-------
>  2 files changed, 67 insertions(+), 23 deletions(-)
> 
> diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h
> index 8612f86511a1..d514aac7697b 100644
> --- a/include/libcamera/file_descriptor.h
> +++ b/include/libcamera/file_descriptor.h
> @@ -14,7 +14,8 @@ namespace libcamera {
>  class FileDescriptor final
>  {
>  public:
> -	explicit FileDescriptor(int fd = -1);
> +	explicit FileDescriptor(const int &fd = -1);
> +	explicit FileDescriptor(int &&fd);
>  	FileDescriptor(const FileDescriptor &other);
>  	FileDescriptor(FileDescriptor &&other);
>  	~FileDescriptor();
> @@ -30,7 +31,7 @@ private:
>  	class Descriptor
>  	{
>  	public:
> -		Descriptor(int fd);
> +		Descriptor(int fd, bool duplicate);
>  		~Descriptor();
>  
>  		int fd() const { return fd_; }
> diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp
> index ee60064bce6e..640e66e6d168 100644
> --- a/src/libcamera/file_descriptor.cpp
> +++ b/src/libcamera/file_descriptor.cpp
> @@ -33,43 +33,81 @@ LOG_DEFINE_CATEGORY(FileDescriptor)
>   * shared with all FileDescriptor instances constructed as copies.
>   *
>   * When constructed from a numerical file descriptor, the FileDescriptor
> - * instance duplicates the file descriptor and wraps the duplicate as a
> - * Descriptor. The copy constructor and assignment operator create copies that
> - * share the Descriptor, while the move versions of those methods additionally
> - * make the other FileDescriptor invalid. When the last FileDescriptor that
> - * references a Descriptor is destroyed, the file descriptor is closed.
> - *
> - * The numerical file descriptor is available through the fd() method. As
> - * constructing a FileDescriptor from a numerical file descriptor duplicates
> - * the file descriptor, the value returned by fd() will be different than the
> - * value passed to the constructor. All FileDescriptor instances created as
> - * copies of a FileDescriptor will report the same fd() value. Callers can
> - * perform operations on the fd(), but shall never close it manually.
> + * instance either duplicates or takes over the file descriptor:
> + *
> + * - The FileDescriptor(const int &) constructor duplicates the numerical file
> + *   descriptor and wraps the duplicate in a Descriptor. The caller is
> + *   responsible for closing the original file descriptor, and the value
> + *   returned by fd() will be different from the value passed to the
> + *   constructor.
> + *
> + * - The FileDescriptor(int &&) constructor takes over the numerical file
> + *   descriptor and wraps it in a Descriptor. The caller is shall not touch the
> + *   original file descriptor once the function returns, and the value returned
> + *   by fd() will be identical to the value passed to the constructor.
> + *
> + * The copy constructor and assignment operator create copies that share the
> + * Descriptor, while the move versions of those methods additionally make the
> + * other FileDescriptor invalid. When the last FileDescriptor that references a
> + * Descriptor is destroyed, the file descriptor is closed.
> + *
> + * The numerical file descriptor is available through the fd() method. All
> + * FileDescriptor instances created as copies of a FileDescriptor will report
> + * the same fd() value. Callers can perform operations on the fd(), but shall
> + * never close it manually.
>   */
>  
>  /**
> - * \brief Create a FileDescriptor wrapping a copy of a given \a fd
> + * \brief Create a FileDescriptor copying a given \a fd
>   * \param[in] fd File descriptor
>   *
> - * Constructing a FileDescriptor from a numerical file descriptor duplicates the
> - * \a fd and takes ownership of the copy. The original \a fd is left untouched,
> - * and the caller is responsible for closing it when appropriate. The duplicated
> - * file descriptor will be closed automatically when all FileDescriptor
> - * instances that reference it are destroyed.
> + * Construct a FileDescriptor from a numerical file descriptor by duplicating
> + * the \a fd, and take ownership of the copy. The original \a fd is left
> + * untouched, and the caller is responsible for closing it when appropriate.
> + * The duplicated file descriptor will be closed automatically when all
> + * FileDescriptor instances that reference it are destroyed.
>   *
>   * If the \a fd is negative, the FileDescriptor is constructed as invalid and
>   * the fd() method will return -1.
>   */
> -FileDescriptor::FileDescriptor(int fd)
> +FileDescriptor::FileDescriptor(const int &fd)
>  {
>  	if (fd < 0)
>  		return;
>  
> -	fd_ = std::make_shared<Descriptor>(fd);
> +	fd_ = std::make_shared<Descriptor>(fd, true);
>  	if (fd_->fd() < 0)
>  		fd_.reset();
>  }
>  
> +/**
> + * \brief Create a FileDescriptor taking ownership of a given \a fd
> + * \param[in] fd File descriptor
> + *
> + * Construct a FileDescriptor from a numerical file descriptor by taking
> + * ownership of the \a fd. The original \a fd is set to -1 and shall not be
> + * touched by the caller anymore. In particular, the caller shall not close the
> + * original \a fd manually. The duplicated file descriptor will be closed
> + * automatically when all FileDescriptor instances that reference it are
> + * destroyed.
> + *
> + * If the \a fd is negative, the FileDescriptor is constructed as invalid and
> + * the fd() method will return -1.
> + */
> +FileDescriptor::FileDescriptor(int &&fd)
> +{
> +	if (fd < 0)
> +		return;
> +
> +	fd_ = std::make_shared<Descriptor>(fd, false);
> +	/*
> +	 * The Descriptor constructor can't have failed here, as it took over
> +	 * the fd without duplicating it. Just set the original fd to -1 to
> +	 * implement move semantics.
> +	 */
> +	fd = -1;
> +}
> +
>  /**
>   * \brief Copy constructor, create a FileDescriptor from a copy of \a other
>   * \param[in] other The other FileDescriptor
> @@ -183,8 +221,13 @@ FileDescriptor FileDescriptor::dup() const
>  	return FileDescriptor(fd());
>  }
>  
> -FileDescriptor::Descriptor::Descriptor(int fd)
> +FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
>  {
> +	if (!duplicate) {
> +		fd_ = fd;
> +		return;
> +	}
> +
>  	/* Failing to dup() a fd should not happen and is fatal. */
>  	fd_ = ::dup(fd);
>  	if (fd_ == -1) {
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

Thank you for the patch.  This does fix the underlying problem for me.

On Mon, 18 May 2020 at 17:48, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The FileDescriptor class, when constructed from a numerical file
> descriptor, duplicates the file descriptor and takes ownership of the
> copy. The caller has to close the original file descriptor manually if
> needed. This is inefficient as the dup() and close() calls could be
> avoided, but can also lead to resource leakage, as recently shown by
> commit 353fc4c22322 ("libcamera: v4l2_videodevice: Fix dangling file
> descriptor").
>
> In an attempt to solve this problem, implement move semantics for the
> FileDescriptor constructor. The constructor taking a numerical file
> descriptor is split in two variants:
>
> - A "fd copy" constructor that takes a const lvalue reference to a
>   numerical file descriptor and duplicates it (corresponding to the
>   current behaviour).
> - A "fd move" constructor that takes a rvalue reference to a numerical
>   file descriptor and takes ownership of it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  include/libcamera/file_descriptor.h |  5 +-
>  src/libcamera/file_descriptor.cpp   | 85 ++++++++++++++++++++++-------
>  2 files changed, 67 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h
> index 8612f86511a1..d514aac7697b 100644
> --- a/include/libcamera/file_descriptor.h
> +++ b/include/libcamera/file_descriptor.h
> @@ -14,7 +14,8 @@ namespace libcamera {
>  class FileDescriptor final
>  {
>  public:
> -       explicit FileDescriptor(int fd = -1);
> +       explicit FileDescriptor(const int &fd = -1);
> +       explicit FileDescriptor(int &&fd);
>         FileDescriptor(const FileDescriptor &other);
>         FileDescriptor(FileDescriptor &&other);
>         ~FileDescriptor();
> @@ -30,7 +31,7 @@ private:
>         class Descriptor
>         {
>         public:
> -               Descriptor(int fd);
> +               Descriptor(int fd, bool duplicate);
>                 ~Descriptor();
>
>                 int fd() const { return fd_; }
> diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp
> index ee60064bce6e..640e66e6d168 100644
> --- a/src/libcamera/file_descriptor.cpp
> +++ b/src/libcamera/file_descriptor.cpp
> @@ -33,43 +33,81 @@ LOG_DEFINE_CATEGORY(FileDescriptor)
>   * shared with all FileDescriptor instances constructed as copies.
>   *
>   * When constructed from a numerical file descriptor, the FileDescriptor
> - * instance duplicates the file descriptor and wraps the duplicate as a
> - * Descriptor. The copy constructor and assignment operator create copies that
> - * share the Descriptor, while the move versions of those methods additionally
> - * make the other FileDescriptor invalid. When the last FileDescriptor that
> - * references a Descriptor is destroyed, the file descriptor is closed.
> - *
> - * The numerical file descriptor is available through the fd() method. As
> - * constructing a FileDescriptor from a numerical file descriptor duplicates
> - * the file descriptor, the value returned by fd() will be different than the
> - * value passed to the constructor. All FileDescriptor instances created as
> - * copies of a FileDescriptor will report the same fd() value. Callers can
> - * perform operations on the fd(), but shall never close it manually.
> + * instance either duplicates or takes over the file descriptor:
> + *
> + * - The FileDescriptor(const int &) constructor duplicates the numerical file
> + *   descriptor and wraps the duplicate in a Descriptor. The caller is
> + *   responsible for closing the original file descriptor, and the value
> + *   returned by fd() will be different from the value passed to the
> + *   constructor.
> + *
> + * - The FileDescriptor(int &&) constructor takes over the numerical file
> + *   descriptor and wraps it in a Descriptor. The caller is shall not touch the
> + *   original file descriptor once the function returns, and the value returned
> + *   by fd() will be identical to the value passed to the constructor.
> + *
> + * The copy constructor and assignment operator create copies that share the
> + * Descriptor, while the move versions of those methods additionally make the
> + * other FileDescriptor invalid. When the last FileDescriptor that references a
> + * Descriptor is destroyed, the file descriptor is closed.
> + *
> + * The numerical file descriptor is available through the fd() method. All
> + * FileDescriptor instances created as copies of a FileDescriptor will report
> + * the same fd() value. Callers can perform operations on the fd(), but shall
> + * never close it manually.
>   */
>
>  /**
> - * \brief Create a FileDescriptor wrapping a copy of a given \a fd
> + * \brief Create a FileDescriptor copying a given \a fd
>   * \param[in] fd File descriptor
>   *
> - * Constructing a FileDescriptor from a numerical file descriptor duplicates the
> - * \a fd and takes ownership of the copy. The original \a fd is left untouched,
> - * and the caller is responsible for closing it when appropriate. The duplicated
> - * file descriptor will be closed automatically when all FileDescriptor
> - * instances that reference it are destroyed.
> + * Construct a FileDescriptor from a numerical file descriptor by duplicating
> + * the \a fd, and take ownership of the copy. The original \a fd is left
> + * untouched, and the caller is responsible for closing it when appropriate.
> + * The duplicated file descriptor will be closed automatically when all
> + * FileDescriptor instances that reference it are destroyed.
>   *
>   * If the \a fd is negative, the FileDescriptor is constructed as invalid and
>   * the fd() method will return -1.
>   */
> -FileDescriptor::FileDescriptor(int fd)
> +FileDescriptor::FileDescriptor(const int &fd)
>  {
>         if (fd < 0)
>                 return;
>
> -       fd_ = std::make_shared<Descriptor>(fd);
> +       fd_ = std::make_shared<Descriptor>(fd, true);
>         if (fd_->fd() < 0)
>                 fd_.reset();
>  }
>
> +/**
> + * \brief Create a FileDescriptor taking ownership of a given \a fd
> + * \param[in] fd File descriptor
> + *
> + * Construct a FileDescriptor from a numerical file descriptor by taking
> + * ownership of the \a fd. The original \a fd is set to -1 and shall not be
> + * touched by the caller anymore. In particular, the caller shall not close the
> + * original \a fd manually. The duplicated file descriptor will be closed
> + * automatically when all FileDescriptor instances that reference it are
> + * destroyed.
> + *
> + * If the \a fd is negative, the FileDescriptor is constructed as invalid and
> + * the fd() method will return -1.
> + */
> +FileDescriptor::FileDescriptor(int &&fd)
> +{
> +       if (fd < 0)
> +               return;
> +
> +       fd_ = std::make_shared<Descriptor>(fd, false);
> +       /*
> +        * The Descriptor constructor can't have failed here, as it took over
> +        * the fd without duplicating it. Just set the original fd to -1 to
> +        * implement move semantics.
> +        */
> +       fd = -1;
> +}
> +
>  /**
>   * \brief Copy constructor, create a FileDescriptor from a copy of \a other
>   * \param[in] other The other FileDescriptor
> @@ -183,8 +221,13 @@ FileDescriptor FileDescriptor::dup() const
>         return FileDescriptor(fd());
>  }
>
> -FileDescriptor::Descriptor::Descriptor(int fd)
> +FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
>  {
> +       if (!duplicate) {
> +               fd_ = fd;
> +               return;
> +       }
> +
>         /* Failing to dup() a fd should not happen and is fatal. */
>         fd_ = ::dup(fd);
>         if (fd_ == -1) {
> --
> Regards,
>
> Laurent Pinchart
>