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

2548 diff mbox series

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@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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

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.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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() ?

> +
> +private:
> +	class Storage

I think we need a better name (I know, I picked this one :-)) in order
to reference this from the documentation. Do you think FD would be a
good class name ? Or Descriptor ? Or something else ?

> +	{
> +	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',

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@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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