[libcamera-devel] [PATCH 2/3] libcamera: Add FileDescriptor to help pass numerical fds around

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 16, 2019 at 01:10:28PM +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/file_descriptor.h |  33 ++++++
>  include/libcamera/meson.build       |   1 +
>  src/libcamera/file_descriptor.cpp   | 158 ++++++++++++++++++++++++++++
>  src/libcamera/meson.build           |   1 +
>  4 files changed, 193 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..e05f111d128e0ab1
> --- /dev/null
> +++ b/include/libcamera/file_descriptor.h
> @@ -0,0 +1,33 @@
> +/* 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__
> +
> +namespace libcamera {
> +
> +class FileDescriptor final
> +{
> +public:
> +	explicit FileDescriptor(int fd = -1);
> +	explicit FileDescriptor(const FileDescriptor &other);
> +	explicit FileDescriptor(FileDescriptor &&other);
> +	~FileDescriptor();
> +
> +	FileDescriptor &operator=(const FileDescriptor &other);
> +	FileDescriptor &operator=(FileDescriptor &&other);
> +
> +	int fd() const { return fd_; }
> +
> +private:
> +	void duplicate(int fd);
> +
> +	int 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..fd87753b456fc3b7
> --- /dev/null
> +++ b/src/libcamera/file_descriptor.cpp
> @@ -0,0 +1,158 @@
> +/* 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 version of those methods move the resource and make the original

s/version/versions/

> + * 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
> + *
> + * 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.
> + */
> +FileDescriptor::FileDescriptor(int fd)
> +	: fd_(-1)
> +{
> +	duplicate(fd);
> +}
> +
> +/**
> + * \brief Copy constructor, create a FileDescriptor from a copy of \a other
> + * \param[in] other The other FileDescriptor
> + *
> + * Constructing 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 the FileDescriptor instance is destroyed.

s/the FileDescriptor instance/this FileDescriptor instance/ to avoid any
ambiguity with \a other ? Same for the move constructor below.

> + */
> +FileDescriptor::FileDescriptor(const FileDescriptor &other)
> +	: FileDescriptor(other.fd_)
> +{
> +}
> +
> +/**
> + * \brief Move constructor, create a FileDescriptor by taking over \a other
> + * \param[in] other The other FileDescriptor
> + *
> + * Constructing a FileDescriptor by taking over a wrapped numerical file

s/Constructing/Construct/

> + * 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 the FileDescriptor instance is destroyed.
> + */
> +FileDescriptor::FileDescriptor(FileDescriptor &&other)
> +	: fd_(utils::exchange(other.fd_, -1))
> +{

You could also have written this

	fd_ = other.fd_;
	other.fd_ = -1;

and similarly below in operator=(), to avoid introducing
std::exchange(). Up to you.

> +}
> +

How about documenting the destructor too ?

/**
 * \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.
 */

Then we could remove the "The file descriptor will be closed
automatically when the FileDescriptor instance is destroyed." sentence
from all constructors, if desired.

> +FileDescriptor::~FileDescriptor()
> +{
> +	if (fd_ != -1)
> +		close(fd_);
> +}
> +
> +/**
> + * \brief Copy assignment operator, replace the wrapped file descriptor with a
> + * duplicate from \a other
> + * \param[in] other The other FileDescriptor
> + *
> + * Close the currently wrapped numerical file descriptor if any and duplicate

s/currently wrapped numerical file descriptor/wrapped file descriptor/ ?
s/ if any/, if any,/ of s/if any/(if any)/

Both comments apply to the move assignment operator below.

> + * the file descriptor from \a other. The original FileDescriptor is left

s/original/\a other/

> + * untouched, and the caller is responsible for closing it when appropriate.

s/closing/destroying/ as FileDescriptor has no close method ?

> + * The duplicated file descriptor will be closed automatically when the
> + * FileDescriptor instance is destroyed.
> + *
> + * \return A reference to the FileDescriptor

s/the FileDescriptor/this FileDescriptor/

> + */
> +FileDescriptor &FileDescriptor::operator=(const FileDescriptor &other)
> +{
> +	if (fd_ != -1)
> +		close(fd_);

I would assign fd_ to -1 here to avoid depending on duplicate() doing
so.

> +
> +	duplicate(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 currently wrapped numerical 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 the FileDescriptor instance is destroyed.
> + *
> + * \return A reference to the FileDescriptor

s/the FileDescriptor/this FileDescriptor/

> + */
> +FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other)
> +{
> +	if (fd_ != -1)
> +		close(fd_);
> +
> +	fd_ = utils::exchange(other.fd_, -1);
> +
> +	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
> + */
> +
> +void FileDescriptor::duplicate(int fd)
> +{
> +	if (fd < 0) {
> +		fd_ = -1;
> +		return;
> +	}

To make this method more self-contained, I think it should either
close(fd_) if fd_ != -1 (removing the close from the copy assignment
operator), or rely on fd_ being == -1 when called, thus removing the fd_
= -1 assignment above. Otherwise duplicate() requires fd_ to be closed
but tolerates it being != -1, which is confusing.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +	/* 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);
> +	}
> +}
> +
> +} /* 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