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

[Jacopo Mondi]

Hi Laurent

On Mon, Nov 29, 2021 at 06:34:41PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Nov 29, 2021 at 03:32:13PM +0100, Jacopo Mondi wrote:
> > On Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart 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>
> > > ---
> > >  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) {
> >
> > As that's only user I'm not sure I get why a static function is better :/
>
> I've considered that, and then thought the function may be useful
> somewhere else in the future. As both Hiro and you have the same
> comment, I'll move it as a static function in this file.
>

I was suggesting to keep it in FileDescriptor, but since we have two
file descriptors now I understand it's not a good idea. Move it
wherever is better !

> > >  				isContiguous = false;
> > >  				break;
> > >  			}
>
> --
> Regards,
>
> Laurent Pinchart