[libcamera-devel,v3,06/17] libcamera: base: file_descriptor: Add constructor from UniqueFD
diff mbox series

Message ID 20211128235752.10836-7-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Introduce UniqueFD
Related show

Commit Message

Laurent Pinchart Nov. 28, 2021, 11:57 p.m. UTC 
From: Hirokazu Honda <hiroh@chromium.org>

Add a FileDescriptor constructor that takes a UniqueFD, transfering
ownership of the file descriptor to the FileDescriptor.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Pass UniqueFD by value
---
 include/libcamera/base/file_descriptor.h |  3 +++
 src/libcamera/base/file_descriptor.cpp   | 13 +++++++++++++
 2 files changed, 16 insertions(+)

Comments

Hirokazu Honda Nov. 29, 2021, 1:45 p.m. UTC | #1 
Hi Laurent, thank you for the patch.

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> From: Hirokazu Honda <hiroh@chromium.org>
>
> Add a FileDescriptor constructor that takes a UniqueFD, transfering
> ownership of the file descriptor to the FileDescriptor.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> ---
> Changes since v2:
>
> - Pass UniqueFD by value
> ---
>  include/libcamera/base/file_descriptor.h |  3 +++
>  src/libcamera/base/file_descriptor.cpp   | 13 +++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
> index 5d1594e80801..74292eba04f5 100644
> --- a/include/libcamera/base/file_descriptor.h
> +++ b/include/libcamera/base/file_descriptor.h
> @@ -11,11 +11,14 @@
>
>  namespace libcamera {
>
> +class UniqueFD;
> +
>  class FileDescriptor final
>  {
>  public:
>         explicit FileDescriptor(const int &fd = -1);
>         explicit FileDescriptor(int &&fd);
> +       explicit FileDescriptor(UniqueFD fd);
>         FileDescriptor(const FileDescriptor &other);
>         FileDescriptor(FileDescriptor &&other);
>         ~FileDescriptor();
> diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
> index 98d4b4bfd24f..da696b2501cd 100644
> --- a/src/libcamera/base/file_descriptor.cpp
> +++ b/src/libcamera/base/file_descriptor.cpp
> @@ -13,6 +13,7 @@
>  #include <utility>
>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/unique_fd.h>
>
>  /**
>   * \file base/file_descriptor.h
> @@ -109,6 +110,18 @@ FileDescriptor::FileDescriptor(int &&fd)
>         fd = -1;
>  }
>
> +/**
> + * \brief Create a FileDescriptor taking ownership of a given UniqueFD \a fd
> + * \param[in] fd UniqueFD
> + *
> + * Construct a FileDescriptor from UniqueFD by taking ownership of the \a fd.
> + * The original \a fd becomes invalid.
> + */
> +FileDescriptor::FileDescriptor(UniqueFD fd)
> +       : FileDescriptor(fd.release())
> +{
> +}
> +
>  /**
>   * \brief Copy constructor, create a FileDescriptor from a copy of \a other
>   * \param[in] other The other FileDescriptor
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi Nov. 29, 2021, 3:08 p.m. UTC | #2 
Hi Laurent

On Mon, Nov 29, 2021 at 01:57:41AM +0200, Laurent Pinchart wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
>
> Add a FileDescriptor constructor that takes a UniqueFD, transfering
> ownership of the file descriptor to the FileDescriptor.
>

Is this a good idea ? Can shared_ptr<>(unique_ptr<>) ?

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
>
> - Pass UniqueFD by value
> ---
>  include/libcamera/base/file_descriptor.h |  3 +++
>  src/libcamera/base/file_descriptor.cpp   | 13 +++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
> index 5d1594e80801..74292eba04f5 100644
> --- a/include/libcamera/base/file_descriptor.h
> +++ b/include/libcamera/base/file_descriptor.h
> @@ -11,11 +11,14 @@
>
>  namespace libcamera {
>
> +class UniqueFD;
> +
>  class FileDescriptor final
>  {
>  public:
>  	explicit FileDescriptor(const int &fd = -1);
>  	explicit FileDescriptor(int &&fd);
> +	explicit FileDescriptor(UniqueFD fd);
>  	FileDescriptor(const FileDescriptor &other);
>  	FileDescriptor(FileDescriptor &&other);
>  	~FileDescriptor();
> diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
> index 98d4b4bfd24f..da696b2501cd 100644
> --- a/src/libcamera/base/file_descriptor.cpp
> +++ b/src/libcamera/base/file_descriptor.cpp
> @@ -13,6 +13,7 @@
>  #include <utility>
>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/unique_fd.h>
>
>  /**
>   * \file base/file_descriptor.h
> @@ -109,6 +110,18 @@ FileDescriptor::FileDescriptor(int &&fd)
>  	fd = -1;
>  }
>
> +/**
> + * \brief Create a FileDescriptor taking ownership of a given UniqueFD \a fd
> + * \param[in] fd UniqueFD
> + *
> + * Construct a FileDescriptor from UniqueFD by taking ownership of the \a fd.
> + * The original \a fd becomes invalid.
> + */
> +FileDescriptor::FileDescriptor(UniqueFD fd)
> +	: FileDescriptor(fd.release())
> +{
> +}
> +
>  /**
>   * \brief Copy constructor, create a FileDescriptor from a copy of \a other
>   * \param[in] other The other FileDescriptor
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Nov. 29, 2021, 4:43 p.m. UTC | #3 
Hi Jacopo,

On Mon, Nov 29, 2021 at 04:08:24PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 29, 2021 at 01:57:41AM +0200, Laurent Pinchart wrote:
> > From: Hirokazu Honda <hiroh@chromium.org>
> >
> > Add a FileDescriptor constructor that takes a UniqueFD, transfering
> > ownership of the file descriptor to the FileDescriptor.
> 
> Is this a good idea ? Can shared_ptr<>(unique_ptr<>) ?

For the second question, yes, see constructor 13 in
https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr. For the
first question, I think so, but we can discuss it :-)

> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Pass UniqueFD by value
> > ---
> >  include/libcamera/base/file_descriptor.h |  3 +++
> >  src/libcamera/base/file_descriptor.cpp   | 13 +++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
> > index 5d1594e80801..74292eba04f5 100644
> > --- a/include/libcamera/base/file_descriptor.h
> > +++ b/include/libcamera/base/file_descriptor.h
> > @@ -11,11 +11,14 @@
> >
> >  namespace libcamera {
> >
> > +class UniqueFD;
> > +
> >  class FileDescriptor final
> >  {
> >  public:
> >  	explicit FileDescriptor(const int &fd = -1);
> >  	explicit FileDescriptor(int &&fd);
> > +	explicit FileDescriptor(UniqueFD fd);
> >  	FileDescriptor(const FileDescriptor &other);
> >  	FileDescriptor(FileDescriptor &&other);
> >  	~FileDescriptor();
> > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
> > index 98d4b4bfd24f..da696b2501cd 100644
> > --- a/src/libcamera/base/file_descriptor.cpp
> > +++ b/src/libcamera/base/file_descriptor.cpp
> > @@ -13,6 +13,7 @@
> >  #include <utility>
> >
> >  #include <libcamera/base/log.h>
> > +#include <libcamera/base/unique_fd.h>
> >
> >  /**
> >   * \file base/file_descriptor.h
> > @@ -109,6 +110,18 @@ FileDescriptor::FileDescriptor(int &&fd)
> >  	fd = -1;
> >  }
> >
> > +/**
> > + * \brief Create a FileDescriptor taking ownership of a given UniqueFD \a fd
> > + * \param[in] fd UniqueFD
> > + *
> > + * Construct a FileDescriptor from UniqueFD by taking ownership of the \a fd.
> > + * The original \a fd becomes invalid.
> > + */
> > +FileDescriptor::FileDescriptor(UniqueFD fd)
> > +	: FileDescriptor(fd.release())
> > +{
> > +}
> > +
> >  /**
> >   * \brief Copy constructor, create a FileDescriptor from a copy of \a other
> >   * \param[in] other The other FileDescriptor
Jacopo Mondi Nov. 29, 2021, 5:23 p.m. UTC | #4 
Hi Laurent,

On Mon, Nov 29, 2021 at 06:43:21PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Nov 29, 2021 at 04:08:24PM +0100, Jacopo Mondi wrote:
> > On Mon, Nov 29, 2021 at 01:57:41AM +0200, Laurent Pinchart wrote:
> > > From: Hirokazu Honda <hiroh@chromium.org>
> > >
> > > Add a FileDescriptor constructor that takes a UniqueFD, transfering
> > > ownership of the file descriptor to the FileDescriptor.
> >
> > Is this a good idea ? Can shared_ptr<>(unique_ptr<>) ?
>
> For the second question, yes, see constructor 13 in
> https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr. For the
> first question, I think so, but we can discuss it :-)
>

Ok then :)
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v2:
> > >
> > > - Pass UniqueFD by value
> > > ---
> > >  include/libcamera/base/file_descriptor.h |  3 +++
> > >  src/libcamera/base/file_descriptor.cpp   | 13 +++++++++++++
> > >  2 files changed, 16 insertions(+)
> > >
> > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
> > > index 5d1594e80801..74292eba04f5 100644
> > > --- a/include/libcamera/base/file_descriptor.h
> > > +++ b/include/libcamera/base/file_descriptor.h
> > > @@ -11,11 +11,14 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class UniqueFD;
> > > +
> > >  class FileDescriptor final
> > >  {
> > >  public:
> > >  	explicit FileDescriptor(const int &fd = -1);
> > >  	explicit FileDescriptor(int &&fd);
> > > +	explicit FileDescriptor(UniqueFD fd);
> > >  	FileDescriptor(const FileDescriptor &other);
> > >  	FileDescriptor(FileDescriptor &&other);
> > >  	~FileDescriptor();
> > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
> > > index 98d4b4bfd24f..da696b2501cd 100644
> > > --- a/src/libcamera/base/file_descriptor.cpp
> > > +++ b/src/libcamera/base/file_descriptor.cpp
> > > @@ -13,6 +13,7 @@
> > >  #include <utility>
> > >
> > >  #include <libcamera/base/log.h>
> > > +#include <libcamera/base/unique_fd.h>
> > >
> > >  /**
> > >   * \file base/file_descriptor.h
> > > @@ -109,6 +110,18 @@ FileDescriptor::FileDescriptor(int &&fd)
> > >  	fd = -1;
> > >  }
> > >
> > > +/**
> > > + * \brief Create a FileDescriptor taking ownership of a given UniqueFD \a fd
> > > + * \param[in] fd UniqueFD
> > > + *
> > > + * Construct a FileDescriptor from UniqueFD by taking ownership of the \a fd.
> > > + * The original \a fd becomes invalid.
> > > + */
> > > +FileDescriptor::FileDescriptor(UniqueFD fd)
> > > +	: FileDescriptor(fd.release())
> > > +{
> > > +}
> > > +
> > >  /**
> > >   * \brief Copy constructor, create a FileDescriptor from a copy of \a other
> > >   * \param[in] other The other FileDescriptor
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
index 5d1594e80801..74292eba04f5 100644
--- a/include/libcamera/base/file_descriptor.h
+++ b/include/libcamera/base/file_descriptor.h
@@ -11,11 +11,14 @@ 
 
 namespace libcamera {
 
+class UniqueFD;
+
 class FileDescriptor final
 {
 public:
 	explicit FileDescriptor(const int &fd = -1);
 	explicit FileDescriptor(int &&fd);
+	explicit FileDescriptor(UniqueFD fd);
 	FileDescriptor(const FileDescriptor &other);
 	FileDescriptor(FileDescriptor &&other);
 	~FileDescriptor();
diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
index 98d4b4bfd24f..da696b2501cd 100644
--- a/src/libcamera/base/file_descriptor.cpp
+++ b/src/libcamera/base/file_descriptor.cpp
@@ -13,6 +13,7 @@ 
 #include <utility>
 
 #include <libcamera/base/log.h>
+#include <libcamera/base/unique_fd.h>
 
 /**
  * \file base/file_descriptor.h
@@ -109,6 +110,18 @@  FileDescriptor::FileDescriptor(int &&fd)
 	fd = -1;
 }
 
+/**
+ * \brief Create a FileDescriptor taking ownership of a given UniqueFD \a fd
+ * \param[in] fd UniqueFD
+ *
+ * Construct a FileDescriptor from UniqueFD by taking ownership of the \a fd.
+ * The original \a fd becomes invalid.
+ */
+FileDescriptor::FileDescriptor(UniqueFD fd)
+	: FileDescriptor(fd.release())
+{
+}
+
 /**
  * \brief Copy constructor, create a FileDescriptor from a copy of \a other
  * \param[in] other The other FileDescriptor