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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Nov 29, 2021 at 06:24:27PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 29, 2021 at 06:45:10PM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 29, 2021 at 04:12:33PM +0100, Jacopo Mondi wrote:
> > > 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 ?
> >
> > I think it would make sense, yes. But that brings a question: which of
> > the two should we use ? :-)
> 
> fd.fd() is awful :)
> Also *_ptr<> have a .get() function, so....

I usually prefer explicit names, but in this case I agree that get() is
likely better. I'll rename fd() to get().

> > > > + * 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);
> >
> > Indeed. I'm tempted to keep the current pattern though, to avoid making
> > this a special case, but if you think it would be better, I'll change
> > it.
> >
> 
> I know, it's a stupid detail, whatever is fine!
> 
> > > > +	}
> > > > +
> > > > +	return UniqueFD(dupFd);
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > >  }
> > > >
> > > >  FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)

-- 
Regards,

Laurent Pinchart