[libcamera-devel] [PATCH v3 03/17] libcamera: base: file_descriptor: Move inode() function to File class

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> The inode() function has always been a bit of an outcast in the
> FileDescriptor class, as it's not related to the core feature provided
> by FileDescriptor, a shared ownership wrapper around file descriptors.
> Move it to the File class instead. It may not be a perfect fit there
> either, but File is a private class, so the inode() function is at least
> not part of the public API anymore.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

I don't have a strong opinion here. But inode() looks to be called in
frame_buffer.cpp, should it be put in frame_buffer.cpp?

Best,
-Hiro
> ---
>  include/libcamera/base/file.h            |  3 +++
>  include/libcamera/base/file_descriptor.h |  3 ---
>  src/libcamera/base/file.cpp              | 23 ++++++++++++++++++++++
>  src/libcamera/base/file_descriptor.cpp   | 25 ------------------------
>  src/libcamera/framebuffer.cpp            |  5 +++--
>  5 files changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h
> index 55e8edd934d4..996751a7ab72 100644
> --- a/include/libcamera/base/file.h
> +++ b/include/libcamera/base/file.h
> @@ -20,6 +20,8 @@
>
>  namespace libcamera {
>
> +class FileDescriptor;
> +
>  class File
>  {
>  public:
> @@ -66,6 +68,7 @@ public:
>         bool unmap(uint8_t *addr);
>
>         static bool exists(const std::string &name);
> +       static ino_t inode(const FileDescriptor &fd);
>
>  private:
>         LIBCAMERA_DISABLE_COPY(File)
> diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h
> index 8d764f8b4a26..5d1594e80801 100644
> --- a/include/libcamera/base/file_descriptor.h
> +++ b/include/libcamera/base/file_descriptor.h
> @@ -8,7 +8,6 @@
>  #pragma once
>
>  #include <memory>
> -#include <sys/types.h>
>
>  namespace libcamera {
>
> @@ -28,8 +27,6 @@ public:
>         int fd() const { return fd_ ? fd_->fd() : -1; }
>         FileDescriptor dup() const;
>
> -       ino_t inode() const;
> -
>  private:
>         class Descriptor
>         {
> diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp
> index ae4be1f95fa6..7043f9461cf7 100644
> --- a/src/libcamera/base/file.cpp
> +++ b/src/libcamera/base/file.cpp
> @@ -14,6 +14,7 @@
>  #include <sys/types.h>
>  #include <unistd.h>
>
> +#include <libcamera/base/file_descriptor.h>
>  #include <libcamera/base/log.h>
>
>  /**
> @@ -472,4 +473,26 @@ bool File::exists(const std::string &name)
>         return !S_ISDIR(st.st_mode);
>  }
>
> +/**
> + * \brief Retrieve the inode of a FileDescriptor
> + *
> + * \return The file descriptor inode on success, or 0 on error
> + */
> +ino_t File::inode(const FileDescriptor &fd)
> +{
> +       if (!fd.isValid())
> +               return 0;
> +
> +       struct stat st;
> +       int ret = fstat(fd.fd(), &st);
> +       if (ret < 0) {
> +               ret = -errno;
> +               LOG(File, Fatal)
> +                       << "Failed to fstat() fd: " << strerror(-ret);
> +               return 0;
> +       }
> +
> +       return st.st_ino;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp
> index f5f87c56eee8..98d4b4bfd24f 100644
> --- a/src/libcamera/base/file_descriptor.cpp
> +++ b/src/libcamera/base/file_descriptor.cpp
> @@ -8,7 +8,6 @@
>  #include <libcamera/base/file_descriptor.h>
>
>  #include <string.h>
> -#include <sys/stat.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  #include <utility>
> @@ -223,30 +222,6 @@ FileDescriptor FileDescriptor::dup() const
>         return FileDescriptor(fd());
>  }
>
> -/**
> - * \brief Retrieve the file descriptor inode
> - *
> - * \todo Should this move to the File class ?
> - *
> - * \return The file descriptor inode on success, or 0 on error
> - */
> -ino_t FileDescriptor::inode() const
> -{
> -       if (!isValid())
> -               return 0;
> -
> -       struct stat st;
> -       int ret = fstat(fd_->fd(), &st);
> -       if (ret < 0) {
> -               ret = -errno;
> -               LOG(FileDescriptor, Fatal)
> -                       << "Failed to fstat() fd: " << strerror(-ret);
> -               return 0;
> -       }
> -
> -       return st.st_ino;
> -}
> -
>  FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
>  {
>         if (!duplicate) {
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 337ea1155a38..f5bcf107d7aa 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -8,6 +8,7 @@
>  #include <libcamera/framebuffer.h>
>  #include "libcamera/internal/framebuffer.h"
>
> +#include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
>
>  /**
> @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>                  */
>                 if (plane.fd.fd() != planes_[0].fd.fd()) {
>                         if (!inode)
> -                               inode = planes_[0].fd.inode();
> -                       if (plane.fd.inode() != inode) {
> +                               inode = File::inode(planes_[0].fd);
> +                       if (File::inode(plane.fd) != inode) {
>                                 isContiguous = false;
>                                 break;
>                         }
> --
> Regards,
>
> Laurent Pinchart
>