[libcamera-devel] [RFC PATCH 01/10] libcamera: ScopedFD: Introduce ScopedFD

[Laurent Pinchart]

Hi Hiro,

Sorry for the very late review, this series got burried in my inbox.

On Thu, Apr 15, 2021 at 05:38:34PM +0900, Hirokazu Honda wrote:
> This introduces ScopedFD. It acts like unique_ptr to a file
> descriptor.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/meson.build              |   1 +
>  include/libcamera/scoped_file_descriptor.h |  38 +++++++
>  src/libcamera/meson.build                  |   1 +
>  src/libcamera/scoped_file_descriptor.cpp   | 119 +++++++++++++++++++++
>  4 files changed, 159 insertions(+)
>  create mode 100644 include/libcamera/scoped_file_descriptor.h
>  create mode 100644 src/libcamera/scoped_file_descriptor.cpp
> 
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index c7b8ee8e..a56f2d26 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -15,6 +15,7 @@ libcamera_public_headers = files([
>      'object.h',
>      'pixel_format.h',
>      'request.h',
> +    'scoped_file_descriptor.h',

Small detail, I'd name this scoped_fd.h to match the class name and
avoid long file names.

>      'signal.h',
>      'span.h',
>      'stream.h',
> diff --git a/include/libcamera/scoped_file_descriptor.h b/include/libcamera/scoped_file_descriptor.h
> new file mode 100644
> index 00000000..baff366a
> --- /dev/null
> +++ b/include/libcamera/scoped_file_descriptor.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * scoped_file_descriptor.h - File descriptor wrapper that owns a file descriptor.
> + */
> +#ifndef __LIBCAMERA_SCOPED_FILE_DESCRIPTOR_H__
> +#define __LIBCAMERA_SCOPED_FILE_DESCRIPTOR_H__
> +
> +#include <libcamera/compiler.h>
> +
> +namespace libcamera {
> +
> +class ScopedFD final
> +{
> +	constexpr static int kInvalidFD = -1;

Blank line.

Isn't it more customary to write "static constexpr" than "constexpr
static" ?

I'm not sure if we really need a named constant as -1 for an invalid
file descriptor is such standard practice that it's very readable, and
kInvalidFD may actually be less readable.

> +public:
> +	explicit ScopedFD(const int fd = kInvalidFD);

Similarly to FileDescriptor, should this function take an rvalue
reference to int fd, in order to set the fd to -1 and ensure it won't be
used by the caller anymore ? We would need a default constructor with no
argument instead of using a default value for fd.

> +	~ScopedFD();
> +	ScopedFD(ScopedFD &&other);
> +	ScopedFD &operator=(ScopedFD &&other);
> +
> +	// Move-only.
> +	ScopedFD(const ScopedFD &) = delete;
> +	ScopedFD &operator=(const ScopedFD &) = delete;

This should use LIBCAMERA_DISABLE_COPY() from class.h.

> +
> +	bool isValid() const { return fd_ != kInvalidFD; }
> +	int get() const { return fd_; }
> +	void reset(const int fd = kInvalidFD);
> +	__nodiscard int release();
> +
> +private:
> +	int fd_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SCOPED_FILE_DESCRIPTOR_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e0a48aa2..6306730a 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -43,6 +43,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'scoped_file_descriptor.cpp',
>      'semaphore.cpp',
>      'signal.cpp',
>      'stream.cpp',
> diff --git a/src/libcamera/scoped_file_descriptor.cpp b/src/libcamera/scoped_file_descriptor.cpp
> new file mode 100644
> index 00000000..1a5b1f9f
> --- /dev/null
> +++ b/src/libcamera/scoped_file_descriptor.cpp
> @@ -0,0 +1,119 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * scoped_file_descriptor.cpp - File descriptor wrapper that owns a file descriptor.
> + */
> +
> +#include <libcamera/scoped_file_descriptor.h>
> +
> +#include <algorithm>

Is this needed ?

> +#include <unistd.h>
> +
> +#include "libcamera/internal/log.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(ScopedFileDescriptor)

You need a \file comment block, or doxygen will not parse the header.

> +
> +/**
> + * \class ScopedFD
> + * \brief unique_ptr like wrapper for a file descriptor.

s/.$//

Same for other \brief lines below.

> + *
> + * The ScopedFD provies RAII-style lifetime management of a file descriptor.

s/provies/provides/

> + * It doesn't allow the shared ownership unlike FileDescriptor. It constructs
> + * with a numerical file descriptor and takes over the file descriptor.

s/It constructs with/It is constructed from/
s/takes over/takes over ownership of/

I'd also add "When the ScopedFD instance is destroyed, the managed file
descriptor is closed.".

> + */
> +
> +/**
> + * \brief Create a ScopedFD taking over a given \a fd.
> + * \param[in] fd a numerical file descriptor.

s/.$// here too (and in some places below)

> + *
> + * Construct a ScopedFD from a numerical file descriptor and take ownership of
> + * the file descriptor. So a caller must not close it. The given file descriptor
> + * is automatically closed when the ScopedFD is destructed.

I'd drop the second sentence.

> + */
> +ScopedFD::ScopedFD(const int fd)
> +	: fd_(fd)
> +{
> +}
> +
> +/**
> + * \brief Destroy the ScopedFD instance
> + *
> + * Destroying a ScopedFD instance. The owned file descriptor is automatically
> + * closed if it is valid.

I'd drop the first sentence, it duplicates the \brief.

> + */
> +ScopedFD::~ScopedFD()
> +{
> +	reset();
> +}
> +
> +/**
> + * \brief Move constructor, create a ScopedFD by taking over \a other
> + * \param[in] other The other ScopedFD.
> + *
> + * Create a ScopedFD with taking the ownership of a file descriptor owned by \a
> + * other.

s/with taking/that takes/
s/a file descriptor/the file descriptor/

And you can add "Upon return, the \a other ScopedFD is invalid."

> + */
> +ScopedFD::ScopedFD(ScopedFD &&other)
> +	: fd_(other.release())
> +{
> +}
> +
> +/**
> + * \brief Move assignment operator, replace a ScopedFD by taking over \a other
> + * \param[in] other The other ScopedFD.
> + *
> + * If a moving ScopedFD has a valid file descriptor, the file descriptor is
> + * closed and then replaced with a file descriptor owned by \a other. As a
> + * result, \a other no longer owns a valid file descriptor.

 * If this ScopedFD has a valid file descriptor, the file descriptor is closed
 * first. The file descriptor is then replaced by the one of \a other. Upon
 * return, \a other is invalid.

> + *
> + * \return A reference to this ScopedFD.

s/.$//

> + */
> +ScopedFD &ScopedFD::operator=(ScopedFD &&other)
> +{
> +	reset(other.release());
> +
> +	return *this;
> +}
> +
> +/**
> + * \fn ScopedFD::isValid()
> + * \brief Check if the ScopedFD has a valid file descriptor.
> + * \return True if the ScopedFD has a valid file descriptor, false otherwise
> + */
> +
> +/**
> + * \fn ScopedFD::get()
> + * \brief Retrieve the numerical file descriptor
> + * \return The numerical file descriptor.
> + */
> +
> +/**
> + * \fn ScopedFD::reset()
> + * \param[in] int a numerical file descriptor.

\param should go after \brief

> + * \brief Swap the owned file descriptor with \a fd. The originally owned file
> + * descriptor is closed.

The brief should be made of a single sentence, otherwise it's not brief
anymore :-) You can split the second sentence to normal documentation
text. Same below.

> + */
> +void ScopedFD::reset(const int fd)
> +{
> +	ASSERT(!isValid() || fd != fd_);
> +	if (isValid())
> +		close(fd_);
> +	fd_ = fd;
> +}
> +
> +/**
> + * \fn ScopedFD::release()
> + * \brief Retrieve a numerical file descriptor. ScopedFD releases the ownership
> + * of it and the caller takes the ownership of it.

The main purpose of this function is to release the file descriptor, so
that's what I would focus on in the brief.

 * \brief Release ownership of the file descriptor without closing it
 *
 * This function releases and returns the owned file descriptor without closing
 * it. The caller owns the returned value and must take care of handling its
 * life time to avoid file descriptor leakages. Upon return the ScopedFD is
 * invalid.

> + * \return A numerical file descriptor.

s/A/The/

Overall this series seems fine, and I think it creates an opportunity to
improve the FileDescriptor class too. I wonder if ScopedFD is the best
name though, it would be nice to create two names for the unique_ptr and
shared_ptr variants (ScopedFD and FileDescriptor) that would reflect
their nature. UniqueFD and SharedFD come to mind, but I'm not sure
they're great.

I'd like to get feedback from others too.

One point to mention, for the sake of it, is that we could also use the
FileDescriptor class instead of ScopedFD. Hiro, why do you think two
separate classes are best ?

> + */
> +int ScopedFD::release()
> +{
> +	int fd = fd_;
> +	fd_ = kInvalidFD;
> +	return fd;
> +}
> +} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart