<div dir="ltr"><div dir="ltr">Hi Laurent, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 4:25 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Sorry for the very late review, this series got burried in my inbox.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:34PM +0900, Hirokazu Honda wrote:<br>
> This introduces ScopedFD. It acts like unique_ptr to a file<br>
> descriptor.<br>
> <br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> include/libcamera/meson.build | 1 +<br>
> include/libcamera/scoped_file_descriptor.h | 38 +++++++<br>
> src/libcamera/meson.build | 1 +<br>
> src/libcamera/scoped_file_descriptor.cpp | 119 +++++++++++++++++++++<br>
> 4 files changed, 159 insertions(+)<br>
> create mode 100644 include/libcamera/scoped_file_descriptor.h<br>
> create mode 100644 src/libcamera/scoped_file_descriptor.cpp<br>
> <br>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build<br>
> index c7b8ee8e..a56f2d26 100644<br>
> --- a/include/libcamera/meson.build<br>
> +++ b/include/libcamera/meson.build<br>
> @@ -15,6 +15,7 @@ libcamera_public_headers = files([<br>
> 'object.h',<br>
> 'pixel_format.h',<br>
> 'request.h',<br>
> + 'scoped_file_descriptor.h',<br>
<br>
Small detail, I'd name this scoped_fd.h to match the class name and<br>
avoid long file names.<br>
<br>
> 'signal.h',<br>
> 'span.h',<br>
> 'stream.h',<br>
> diff --git a/include/libcamera/scoped_file_descriptor.h b/include/libcamera/scoped_file_descriptor.h<br>
> new file mode 100644<br>
> index 00000000..baff366a<br>
> --- /dev/null<br>
> +++ b/include/libcamera/scoped_file_descriptor.h<br>
> @@ -0,0 +1,38 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2021, Google Inc.<br>
> + *<br>
> + * scoped_file_descriptor.h - File descriptor wrapper that owns a file descriptor.<br>
> + */<br>
> +#ifndef __LIBCAMERA_SCOPED_FILE_DESCRIPTOR_H__<br>
> +#define __LIBCAMERA_SCOPED_FILE_DESCRIPTOR_H__<br>
> +<br>
> +#include <libcamera/compiler.h><br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +class ScopedFD final<br>
> +{<br>
> + constexpr static int kInvalidFD = -1;<br>
<br>
Blank line.<br>
<br>
Isn't it more customary to write "static constexpr" than "constexpr<br>
static" ?<br>
<br>
I'm not sure if we really need a named constant as -1 for an invalid<br>
file descriptor is such standard practice that it's very readable, and<br>
kInvalidFD may actually be less readable.<br>
<br>
> +public:<br>
> + explicit ScopedFD(const int fd = kInvalidFD);<br>
<br>
Similarly to FileDescriptor, should this function take an rvalue<br>
reference to int fd, in order to set the fd to -1 and ensure it won't be<br>
used by the caller anymore ? We would need a default constructor with no<br>
argument instead of using a default value for fd.<br>
<br>
> + ~ScopedFD();<br>
> + ScopedFD(ScopedFD &&other);<br>
> + ScopedFD &operator=(ScopedFD &&other);<br>
> +<br>
> + // Move-only.<br>
> + ScopedFD(const ScopedFD &) = delete;<br>
> + ScopedFD &operator=(const ScopedFD &) = delete;<br>
<br>
This should use LIBCAMERA_DISABLE_COPY() from class.h.<br>
<br>
> +<br>
> + bool isValid() const { return fd_ != kInvalidFD; }<br>
> + int get() const { return fd_; }<br>
> + void reset(const int fd = kInvalidFD);<br>
> + __nodiscard int release();<br>
> +<br>
> +private:<br>
> + int fd_;<br>
> +};<br>
> +<br>
> +} /* namespace libcamera */<br>
> +<br>
> +#endif /* __LIBCAMERA_SCOPED_FILE_DESCRIPTOR_H__ */<br>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
> index e0a48aa2..6306730a 100644<br>
> --- a/src/libcamera/meson.build<br>
> +++ b/src/libcamera/meson.build<br>
> @@ -43,6 +43,7 @@ libcamera_sources = files([<br>
> 'process.cpp',<br>
> 'pub_key.cpp',<br>
> 'request.cpp',<br>
> + 'scoped_file_descriptor.cpp',<br>
> 'semaphore.cpp',<br>
> 'signal.cpp',<br>
> 'stream.cpp',<br>
> diff --git a/src/libcamera/scoped_file_descriptor.cpp b/src/libcamera/scoped_file_descriptor.cpp<br>
> new file mode 100644<br>
> index 00000000..1a5b1f9f<br>
> --- /dev/null<br>
> +++ b/src/libcamera/scoped_file_descriptor.cpp<br>
> @@ -0,0 +1,119 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2021, Google Inc.<br>
> + *<br>
> + * scoped_file_descriptor.cpp - File descriptor wrapper that owns a file descriptor.<br>
> + */<br>
> +<br>
> +#include <libcamera/scoped_file_descriptor.h><br>
> +<br>
> +#include <algorithm><br>
<br>
Is this needed ?<br>
<br>
> +#include <unistd.h><br>
> +<br>
> +#include "libcamera/internal/log.h"<br>
> +<br>
> +namespace libcamera {<br>
> +<br>
> +LOG_DECLARE_CATEGORY(ScopedFileDescriptor)<br>
<br>
You need a \file comment block, or doxygen will not parse the header.<br>
<br>
> +<br>
> +/**<br>
> + * \class ScopedFD<br>
> + * \brief unique_ptr like wrapper for a file descriptor.<br>
<br>
s/.$//<br>
<br>
Same for other \brief lines below.<br>
<br>
> + *<br>
> + * The ScopedFD provies RAII-style lifetime management of a file descriptor.<br>
<br>
s/provies/provides/<br>
<br>
> + * It doesn't allow the shared ownership unlike FileDescriptor. It constructs<br>
> + * with a numerical file descriptor and takes over the file descriptor.<br>
<br>
s/It constructs with/It is constructed from/<br>
s/takes over/takes over ownership of/<br>
<br>
I'd also add "When the ScopedFD instance is destroyed, the managed file<br>
descriptor is closed.".<br>
<br>
> + */<br>
> +<br>
> +/**<br>
> + * \brief Create a ScopedFD taking over a given \a fd.<br>
> + * \param[in] fd a numerical file descriptor.<br>
<br>
s/.$// here too (and in some places below)<br>
<br>
> + *<br>
> + * Construct a ScopedFD from a numerical file descriptor and take ownership of<br>
> + * the file descriptor. So a caller must not close it. The given file descriptor<br>
> + * is automatically closed when the ScopedFD is destructed.<br>
<br>
I'd drop the second sentence.<br>
<br>
> + */<br>
> +ScopedFD::ScopedFD(const int fd)<br>
> + : fd_(fd)<br>
> +{<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Destroy the ScopedFD instance<br>
> + *<br>
> + * Destroying a ScopedFD instance. The owned file descriptor is automatically<br>
> + * closed if it is valid.<br>
<br>
I'd drop the first sentence, it duplicates the \brief.<br>
<br>
> + */<br>
> +ScopedFD::~ScopedFD()<br>
> +{<br>
> + reset();<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Move constructor, create a ScopedFD by taking over \a other<br>
> + * \param[in] other The other ScopedFD.<br>
> + *<br>
> + * Create a ScopedFD with taking the ownership of a file descriptor owned by \a<br>
> + * other.<br>
<br>
s/with taking/that takes/<br>
s/a file descriptor/the file descriptor/<br>
<br>
And you can add "Upon return, the \a other ScopedFD is invalid."<br>
<br>
> + */<br>
> +ScopedFD::ScopedFD(ScopedFD &&other)<br>
> + : fd_(other.release())<br>
> +{<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Move assignment operator, replace a ScopedFD by taking over \a other<br>
> + * \param[in] other The other ScopedFD.<br>
> + *<br>
> + * If a moving ScopedFD has a valid file descriptor, the file descriptor is<br>
> + * closed and then replaced with a file descriptor owned by \a other. As a<br>
> + * result, \a other no longer owns a valid file descriptor.<br>
<br>
* If this ScopedFD has a valid file descriptor, the file descriptor is closed<br>
* first. The file descriptor is then replaced by the one of \a other. Upon<br>
* return, \a other is invalid.<br>
<br>
> + *<br>
> + * \return A reference to this ScopedFD.<br>
<br>
s/.$//<br>
<br>
> + */<br>
> +ScopedFD &ScopedFD::operator=(ScopedFD &&other)<br>
> +{<br>
> + reset(other.release());<br>
> +<br>
> + return *this;<br>
> +}<br>
> +<br>
> +/**<br>
> + * \fn ScopedFD::isValid()<br>
> + * \brief Check if the ScopedFD has a valid file descriptor.<br>
> + * \return True if the ScopedFD has a valid file descriptor, false otherwise<br>
> + */<br>
> +<br>
> +/**<br>
> + * \fn ScopedFD::get()<br>
> + * \brief Retrieve the numerical file descriptor<br>
> + * \return The numerical file descriptor.<br>
> + */<br>
> +<br>
> +/**<br>
> + * \fn ScopedFD::reset()<br>
> + * \param[in] int a numerical file descriptor.<br>
<br>
\param should go after \brief<br>
<br>
> + * \brief Swap the owned file descriptor with \a fd. The originally owned file<br>
> + * descriptor is closed.<br>
<br>
The brief should be made of a single sentence, otherwise it's not brief<br>
anymore :-) You can split the second sentence to normal documentation<br>
text. Same below.<br>
<br>
> + */<br>
> +void ScopedFD::reset(const int fd)<br>
> +{<br>
> + ASSERT(!isValid() || fd != fd_);<br>
> + if (isValid())<br>
> + close(fd_);<br>
> + fd_ = fd;<br>
> +}<br>
> +<br>
> +/**<br>
> + * \fn ScopedFD::release()<br>
> + * \brief Retrieve a numerical file descriptor. ScopedFD releases the ownership<br>
> + * of it and the caller takes the ownership of it.<br>
<br>
The main purpose of this function is to release the file descriptor, so<br>
that's what I would focus on in the brief.<br>
<br>
* \brief Release ownership of the file descriptor without closing it<br>
*<br>
* This function releases and returns the owned file descriptor without closing<br>
* it. The caller owns the returned value and must take care of handling its<br>
* life time to avoid file descriptor leakages. Upon return the ScopedFD is<br>
* invalid.<br>
<br>
> + * \return A numerical file descriptor.<br>
<br>
s/A/The/<br>
<br>
Overall this series seems fine, and I think it creates an opportunity to<br>
improve the FileDescriptor class too. I wonder if ScopedFD is the best<br>
name though, it would be nice to create two names for the unique_ptr and<br>
shared_ptr variants (ScopedFD and FileDescriptor) that would reflect<br>
their nature. UniqueFD and SharedFD come to mind, but I'm not sure<br>
they're great.<br>
<br>
I'd like to get feedback from others too.<br>
<br>
One point to mention, for the sake of it, is that we could also use the<br>
FileDescriptor class instead of ScopedFD. Hiro, why do you think two<br>
separate classes are best ?<br>
<br></blockquote><div><br></div><div>As you know, ScopedFD is like unique_ptr and FileDescriptor is like shared_ptr.</div><div>It is possible to always use shared_ptr, but using unique_ptr clarifies the ownership.</div><div>From my experience, it is rare that a shared ownership class like FileDescriptor has to be used.</div><div>I think I could show it through this patch series.</div><div><br></div><div>-Hiro </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + */<br>
> +int ScopedFD::release()<br>
> +{<br>
> + int fd = fd_;<br>
> + fd_ = kInvalidFD;<br>
> + return fd;<br>
> +}<br>
> +} /* namespace libcamera */<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>