[libcamera-devel] [RFC PATCH v1 02/12] libcamera: file_descriptor: Add a function to retrieve the inode

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote:
> > On 02/09/2021 05:22, Laurent Pinchart wrote:
> > > The inode is useful to check if two file descriptors refer to the same
> > > file. Add a function to retrieve it.
> >
> > Aha, great, and I expect this works on virtual-fs so dma-bufs will be
> > comparable?
>
> That's the goal, yes.
>
> Dmabuf instances are associated with an inode, so two different file
> descriptors that have the same inode are guaranteed to refer to the same
> dmabuf (provided the file descriptors reference a dmabuf, inodes are not
> unique globally). This works with dup() or when sending the file
> descriptor over an IPC mechanism.
>
> > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)
>
> For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer
> will create multiple distinct dmabuf instances, and there's no way for
> userspace to know that they related to the same underlying buffer.
> Buffers allocated by DRM doesn't suffer from this issue, the always
> return the same dmabuf when exporting the buffer multiple times. This
> should be fixable on the V4L2 side.
>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  include/libcamera/file_descriptor.h |  3 +++
> > >  src/libcamera/file_descriptor.cpp   | 22 ++++++++++++++++++++++
> > >  2 files changed, 25 insertions(+)
> > >
> > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h
> > > index d514aac7697b..988f9b7a3d25 100644
> > > --- a/include/libcamera/file_descriptor.h
> > > +++ b/include/libcamera/file_descriptor.h
> > > @@ -8,6 +8,7 @@
> > >  #define __LIBCAMERA_FILE_DESCRIPTOR_H__
> > >
> > >  #include <memory>
> > > +#include <sys/types.h>
> > >
> > >  namespace libcamera {
> > >
> > > @@ -27,6 +28,8 @@ public:
> > >     int fd() const { return fd_ ? fd_->fd() : -1; }
> > >     FileDescriptor dup() const;
> > >
> > > +   ino_t inode() const;
> > > +
> > >  private:
> > >     class Descriptor
> > >     {
> > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp
> > > index 9f9ebc81f738..d55e2b100b6d 100644
> > > --- a/src/libcamera/file_descriptor.cpp
> > > +++ b/src/libcamera/file_descriptor.cpp
> > > @@ -8,6 +8,8 @@
> > >  #include <libcamera/file_descriptor.h>
> > >
> > >  #include <string.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/types.h>
> > >  #include <unistd.h>
> > >  #include <utility>
> > >
> > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const
> > >     return FileDescriptor(fd());
> > >  }
> > >
> > > +/**
> > > + * \brief Retrieve the file descriptor inode
> > > + *
> > > + * \todo Should this move to the File class ?
> >
> > I don't think so.
> >
> > File could implement an operator== which checks that the inode of both
> > is the same?
> >
> > Two File()'s with the same inode are the same, but two FileDescriptors
> > have ... separate file descriptors, so while they point to the same
> > location, they are not the same...
>
> It depends how you define "file" :-) Calling open() twice on the same
> path (thus referring to the same inode) will create two files on the
> kernel side, each of them maintaining their own context (such as the
> read/write pointer and other flags).
>
> We could compare File instances based on different underlying objects,
> including
>
> - The path (with or without following symlinks)
> - The kernel file instance (not sure how though, kcmp() may be an option)
> - The inode
>
> Note that inode comparison isn't enough, as they're not globally unique.
> They are usually unique within a file system (so we can compare the stat
> st_dev and st_ino fields), but that's not always guaranteed (on btrfs
> for instance, see https://lwn.net/Articles/866582/).
>
> > I guess a FileDescriptor is just a pointer, and the inode is the pointer
> > value ;-)
>
> There are three levels, inode (within the context of a filesystem), file
> (from a kernel point of view) and file descriptor. You can have multiple
> files referring to the same inode (for instance by open()ing the same
> path twice), and multiple file descriptors referring to the same file
> (for instance by calling dup() or sending the file descriptor over IPC).
>
> So FileDescriptor is indeed a pointer, but it's the top of the stack,
> while the inode is at the bottom.
>
> > > + *
> > > + * \return The file descriptor inode on success, or 0 on error
> > > + */
> > > +ino_t FileDescriptor::inode() const
> > > +{
> > > +   if (!fd_ || fd_->fd() == -1)
> > > +           return 0;

if (!isValid())
  return 0;

As far as I look the FileDescriptor code, if isValid() is true, fd_
must be valid.

> > > +
> > > +   struct stat st;
> > > +   int ret = fstat(fd_->fd(), &st);
> > > +   if (ret < 0)
> > > +           return 0;

Shall we add the log?
LOG(FileDescriptor, Error) << "fstat failed: " << errno;

> > > +
> > > +   return st.st_ino;

I am a bit concerned two invalid file descriptors are regarded thanks
to returning 0.
Perhaps, let the return type be int64_t and cast ino_t to int64_t?
Or since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine),
shall we return pair(ino_t, error) or error + pointer argument ino_t?

> >
> > I'm torn ... we could parse this when we first construct, and cache the
> > inode.
>
> I didn't to avoid the overhead that is not required in the majority of
> use cases.
>
> > That would save the system call on later comparisons, which may occur
> > more than once on some FileDescriptors, but ... never on others.
> >
> >
> > Or perhaps, as the inode can't be changed underneath the fd, it could
> > simply be cached on the first call ...
>
> I've thought about that too, but wasn't very keen on adding a field to
> the FileDescriptor class for internal purpose only, as inode() isn't
> meant to be a public API. If FileDescriptor was moved to base/ that may
> be different, then we could consider which features belong to File and
> which belong to FileDescriptor, and create a nice API. For now, I've
> decided to just add FileDescriptor::inode() without caching, and add a
> \todo.
>
> One option, if we decide to keep this in FileDescriptor, is to cache it
> in FileDescriptor::Descriptor.
>
> > Do you see any value in putting an operator== in here? It seems
> > reasonable to say that if two FileDescriptors have the same inode, they
> > are == ... however - they may of course be dup()s, with different FDs so
> > we might still want to distinguish that..
>
> I think an operator==() for both File and FileDescriptor would make
> sense, but again we need to define what equality means first :-) I'd
> rather not do so as part of this series, which I want to merge fast to
> fixes breakages in the master branch. That's why there's a todo at the
> top of the patch, I think we can review the design of File and
> FileDescriptor later.
>

I could not imagine the use case of comparing File.
For FileDescriptors, it might be useful, I would use just inode
comparison manually on a caller side because currently the use cases
are small enough (android/camera_device and libcamera/v4l2_device
only?).

Best Regards,
-Hiro
> > Anyway, a lot of that is dependant upon how it ends up getting used...
> >
> > I think it's a good thing to add.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > +}
> > > +
> > >  FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
> > >  {
> > >     if (!duplicate) {
>
> --
> Regards,
>
> Laurent Pinchart