[libcamera-devel] [RFC PATCH 01/10] libcamera: ScopedFD: Introduce ScopedFD
Hirokazu Honda
hiroh at chromium.org
Mon Jun 7 00:12:33 CEST 2021
Hi Laurent, thank you for reviewing.
On Mon, Jun 7, 2021 at 4:25 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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 ?
>
>
As you know, ScopedFD is like unique_ptr and FileDescriptor is like
shared_ptr.
It is possible to always use shared_ptr, but using unique_ptr clarifies the
ownership.
>From my experience, it is rare that a shared ownership class like
FileDescriptor has to be used.
I think I could show it through this patch series.
-Hiro
> > + */
> > +int ScopedFD::release()
> > +{
> > + int fd = fd_;
> > + fd_ = kInvalidFD;
> > + return fd;
> > +}
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210607/b70e7254/attachment.htm>
More information about the libcamera-devel
mailing list