[libcamera-devel] [PATCH 04/30] libcamera: buffer: Add FileDecriptor to help deal with file descriptors

[Laurent Pinchart]

Hello,

On Wed, Nov 27, 2019 at 11:52:04AM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:35:54AM +0100, Niklas Söderlund wrote:
> > Add a helper to make it easier to pass file descriptors around. The
> > helper class duplicates the fd which decouples it from the original fd
> > which could be closed by its owner while the new FileDescriptor remains
> > valid.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/buffer.h | 12 ++++++++++++
> >  src/libcamera/buffer.cpp   | 40 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 7302b9f32ce8c50d..33793b4ccf881eda 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -46,6 +46,18 @@ private:
> >  	std::vector<Plane> planes_;
> >  };
> >
> > +class FileDescriptor final
> > +{
> > +public:
> > +	FileDescriptor(int fd);
> > +	~FileDescriptor();

You're missing the copy constructor and assignment operator that should
dup() the fd, and the move versions that should set them to -1 in the
original.

All constructors should be marked as explicit to avoid unwanted copies,
as those are a bit expensive.

> > +
> > +	int fd() const { return fd_; }
> > +
> > +private:
> > +	int fd_;
> > +};
> > +
> >  class Plane final
> >  {
> >  public:
> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > index 4acff216cafba484..0676586ae3be2a61 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -115,6 +115,46 @@ void BufferInfo::update(Status status, unsigned int sequence,
> >   * \return A array holding all plane information for the buffer
> >   */
> >
> > +/**
> > + * \class FileDescriptor
> > + * \brief Life time management of a file descriptor
> > + *
> > + * The managed file descriptor (fd) is duplicated from the original one and
> > + * is opened for the life time of the FileDescriptor object disregarding
> 
> s/is opened/stays opened/ ?
> 
> > + * if the original one is closed.

 * \brief RAII-style wrapper for file descriptors
 *
 * The FileDescriptor class wraps a file descriptor (expressed as a signed
 * integer) to provide an RAII-style mechanism for owning the file descriptor.
 * When constructed, the FileDescriptor instance duplicates a given file
 * descriptor and takes ownership of the duplicate as a resource. The copy
 * constructor and assignment operator duplicate the file descriptor, while the
 * move version of those methods move the resource and make the original
 * FileDescriptor invalid. When the FileDescriptor is deleted, it closes the
 * file descriptor it owns, if any.

(See https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization
for a definition of RAII)

> > + */
> > +
> > +/**
> > + * \brief Create a life time managed file descriptor
> > + * \param[in] fd File descriptor

 * \brief Create a FileDescriptor wrapping a copy of 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 the FileDescriptor instance
 * is destroyed.

I wonder if we should also define a

FileDescriptor::FileDescriptor(int &&fd)

that skips the duplication, as the caller may sometimes not have a need
to keep the original around.

> > + */
> > +FileDescriptor::FileDescriptor(int fd)
> > +	: fd_(-1)
> > +{
> > +	if (fd < 0)
> > +		return;
> 
> If one has gone so fare as providing an invalid fd to the constructor,
> probably has missed some error check, I would LOG() an error here (or
> even Fatal ? )

There could be use cases for creating FileDescriptor instances that
don't own any file descriptor, and then assigning a valid instance to
them with operator=(). I would this not add any check here, and set a
default value of -1 for the fd parameter in the header file.

> > +
> > +	/* Failing to dup() a fd should not happen and is fatal. */
> > +	fd_ = dup(fd);
> > +	if (fd_ == -1) {
> > +		int ret = -errno;
> > +		LOG(Buffer, Fatal)
> > +			<< "Failed to dup() fd: " << strerror(-ret);
> > +	}
> > +}
> > +
> > +FileDescriptor::~FileDescriptor()
> > +{
> > +	if (fd_ != -1)
> > +		close(fd_);
> > +}
> > +
> > +/**
> > + * \fn FileDescriptor::fd()
> > + * \brief Retrieve the file descriptor or
> 
> or.. ?
> 
> > + * \return A valid file descriptor or a negative error code
> 
> Not a negative error code, but -1 if the instance has been created
> with an invalid file descriptor. How to put it nicely here, well...
> not sure :)

 * \brief Retrieve the numerical file descriptor
 * \return The numerical file descriptor, which may be -1 if the FileDescriptor
 * instance doesn't own a file descriptor

> 
> Wording apart:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > + */
> > +
> >  /**
> >   * \class Plane
> >   * \brief A memory region to store a single plane of a frame

-- 
Regards,

Laurent Pinchart