[libcamera-devel] [PATCH v3 01/33] libcamera: Add FileDescriptor to help pass numerical fds around

Sat Jan 11 00:15:05 CET 2020

Hi Laurent,

Thanks for your feedback.

On 2020-01-11 00:55:49 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 10, 2020 at 08:37:36PM +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.
> 
> The documentation is now out of sync with the code. I'll provide an
> updated version of this patch.

Thanks :-)

More comments bellow.

> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/file_descriptor.h |  46 ++++++++
> >  include/libcamera/meson.build       |   1 +
> >  src/libcamera/file_descriptor.cpp   | 170 ++++++++++++++++++++++++++++
> >  src/libcamera/meson.build           |   1 +
> >  4 files changed, 218 insertions(+)
> >  create mode 100644 include/libcamera/file_descriptor.h
> >  create mode 100644 src/libcamera/file_descriptor.cpp
> > 
> > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h
> > new file mode 100644
> > index 0000000000000000..f08c105998cc7559
> > --- /dev/null
> > +++ b/include/libcamera/file_descriptor.h
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * file_descriptor.h - File descriptor wrapper
> > + */
> > +#ifndef __LIBCAMERA_FILE_DESCRIPTOR_H__
> > +#define __LIBCAMERA_FILE_DESCRIPTOR_H__
> > +
> > +#include <memory>
> > +
> > +namespace libcamera {
> > +
> > +class FileDescriptor final
> > +{
> > +public:
> > +	explicit FileDescriptor(int fd = -1);
> > +	FileDescriptor(const FileDescriptor &other);
> > +	FileDescriptor(FileDescriptor &&other);
> > +	~FileDescriptor();
> > +
> > +	FileDescriptor &operator=(const FileDescriptor &other);
> > +	FileDescriptor &operator=(FileDescriptor &&other);
> > +
> > +	int fd() const { return fd_ ? fd_->fd() : -1; }
> > +	FileDescriptor dup() const;
> 
> Do you think dup() is a good name, or should this be called duplicate()
> ? Or clone() ?

I rather like dup() since this is a low level wrapper around a fd so it 
feels natural to keep to operation names close to the syscalls.  But I 
have no strong feeling, duplicate() and clone() also conveys the intent.

> 
> > +
> > +private:
> > +	class Storage
> 
> I think we need a better name (I know, I picked this one :-))

:-P

> in order
> to reference this from the documentation. Do you think FD would be a
> good class name ? Or Descriptor ? Or something else ?

I rather liked FileDescriptor::Storage as it clearly shows this is the 
shared storage. I'm fine with Descriptor too, but FD feels wrong.

> 
> > +	{
> > +	public:
> > +		Storage(int fd);
> > +		~Storage();
> > +
> > +		int fd() const { return fd_; }
> > +
> > +	private:
> > +		int fd_;
> > +	};
> > +
> > +	std::shared_ptr<Storage> fd_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_FILE_DESCRIPTOR_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 99abf06099407c1f..543e6773cc5158a0 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -6,6 +6,7 @@ libcamera_api = files([
> >      'controls.h',
> >      'event_dispatcher.h',
> >      'event_notifier.h',
> > +    'file_descriptor.h',
> >      'geometry.h',
> >      'logging.h',
> >      'object.h',
> > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp
> > new file mode 100644
> > index 0000000000000000..2e531f40696be16c
> > --- /dev/null
> > +++ b/src/libcamera/file_descriptor.cpp
> > @@ -0,0 +1,170 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * file_descriptor.cpp - File descriptor wrapper
> > + */
> > +
> > +#include <libcamera/file_descriptor.h>
> > +
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <utility>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file file_descriptor.h
> > + * \brief File descriptor wrapper
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(FileDescriptor)
> > +
> > +/**
> > + * \class FileDescriptor
> > + * \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 versions 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.
> > + */
> > +
> > +/**
> > + * \brief Create a FileDescriptor wrapping a copy of a given \a fd
> > + * \param[in] fd File descriptor
> > + *
> > + * Construct 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.
> > + */
> > +FileDescriptor::FileDescriptor(int fd)
> > +{
> > +	fd_ = std::make_shared<Storage>(fd);
> > +}
> > +
> > +/**
> > + * \brief Copy constructor, create a FileDescriptor from a copy of \a other
> > + * \param[in] other The other FileDescriptor
> > + *
> > + * Construct a FileDescriptor from another FileDescriptor duplicates the
> > + * wrapped numerical file descriptor and takes ownership of the copy. The
> > + * original FileDescriptor is left untouched, and the caller is responsible for
> > + * closing it when appropriate. The duplicated file descriptor will be closed
> > + * automatically when this FileDescriptor instance is destroyed.
> > + */
> > +FileDescriptor::FileDescriptor(const FileDescriptor &other)
> > +	: fd_(other.fd_)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Move constructor, create a FileDescriptor by taking over \a other
> > + * \param[in] other The other FileDescriptor
> > + *
> > + * Construct a FileDescriptor by taking over a wrapped numerical file
> > + * descriptor and taking ownership of it. The \a other FileDescriptor is
> > + * invalidated and set to -1. The taken over file descriptor will be closed
> > + * automatically when this FileDescriptor instance is destroyed.
> > + */
> > +FileDescriptor::FileDescriptor(FileDescriptor &&other)
> > +	: fd_(std::move(other.fd_))
> > +{
> > +}
> > +
> > +/**
> > + * \brief Destroy the managed file descriptor
> > + *
> > + * If the managed file descriptor, as returned by fd(), is not equal to -1, the
> > + * file descriptor is closed.
> > + */
> > +FileDescriptor::~FileDescriptor()
> > +{
> > +}
> > +
> > +/**
> > + * \brief Copy assignment operator, replace the wrapped file descriptor with a
> > + * duplicate from \a other
> > + * \param[in] other The other FileDescriptor
> > + *
> > + * Close the wrapped file descriptor (if any) and duplicate the file descriptor
> > + * from \a other. The \a other FileDescriptor is left untouched, and the caller
> > + * is responsible for destroying it when appropriate. The duplicated file
> > + * descriptor will be closed automatically when this FileDescriptor instance is
> > + * destroyed.
> > + *
> > + * \return A reference to this FileDescriptor
> > + */
> > +FileDescriptor &FileDescriptor::operator=(const FileDescriptor &other)
> > +{
> > +	fd_ = other.fd_;
> > +
> > +	return *this;
> > +}
> > +
> > +/**
> > + * \brief Move assignment operator, replace the wrapped file descriptor by
> > + * taking over \a other
> > + * \param[in] other The other FileDescriptor
> > + *
> > + * Close the wrapped file descriptor (if any) and take over the file descriptor
> > + * from \a other. The \a other FileDescriptor is invalidated and set to -1. The
> > + * taken over file descriptor will be closed automatically when this
> > + * FileDescriptor instance is destroyed.
> > + *
> > + * \return A reference to this FileDescriptor
> > + */
> > +FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other)
> > +{
> > +	fd_ = std::move(other.fd_);
> > +
> > +	return *this;
> > +}
> > +
> > +/**
> > + * \fn FileDescriptor::fd()
> > + * \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
> > + */
> > +
> > +/**
> > + * \brief Duplicate a FileDescriptor
> > + * \return A new FileDescriptor instance wrapping a duplicate of the original
> > + * file descriptor
> > + */
> > +FileDescriptor FileDescriptor::dup() const
> > +{
> > +	return FileDescriptor(fd_ ? fd_->fd() : -1);
> > +}
> > +
> > +FileDescriptor::Storage::Storage(int fd)
> > +	: fd_(-1)
> > +{
> > +	if (fd < 0)
> > +		return;
> > +
> > +	/* Failing to dup() a fd should not happen and is fatal. */
> > +	fd_ = ::dup(fd);
> > +	if (fd_ == -1) {
> > +		int ret = -errno;
> > +		LOG(FileDescriptor, Fatal)
> > +			<< "Failed to dup() fd: " << strerror(-ret);
> > +	}
> > +}
> > +
> > +FileDescriptor::Storage::~Storage()
> > +{
> > +	if (fd_ != -1)
> > +		close(fd_);
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index c4f965bd7413b37e..722c5bc15afe52ef 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -14,6 +14,7 @@ libcamera_sources = files([
> >      'event_dispatcher.cpp',
> >      'event_dispatcher_poll.cpp',
> >      'event_notifier.cpp',
> > +    'file_descriptor.cpp',
> >      'formats.cpp',
> >      'geometry.cpp',
> >      'ipa_context_wrapper.cpp',
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund