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

[Laurent Pinchart]

Hi Hiro,

On Mon, Nov 29, 2021 at 11:05:51PM +0900, Hirokazu Honda wrote:
> On Mon, Nov 29, 2021 at 8:58 AM 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>
> 
> I think this is a correct change.
> Although I have no idea when dup() should be used.
> In fact, there seems to be no code calling FileDescriptor::dup().
> Do you have any idea?

You're right, it's not used. There's a place where it would be useful
though, I'll give it a try in v4.

> > ---
> >  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
> > + * 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;
> > +               LOG(FileDescriptor, Error)
> > +                       << "Failed to dup() fd: " << strerror(-ret);
> > +       }
> > +
> > +       return UniqueFD(dupFd);
> >  }
> >
> >  FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)

-- 
Regards,

Laurent Pinchart