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

[Hirokazu Honda]

Hi Laurent,

On Fri, Sep 3, 2021 at 7:35 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Sep 03, 2021 at 07:07:47PM +0900, Hirokazu Honda wrote:
> > On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart wrote:
> > > 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.
>
> Good point, I'll do that.
>
> > > > > +
> > > > > +   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;
>
> I'll add an error message in v2.
>
> > > > > +
> > > > > +   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?
>
> 0 is never allocated as a valid inode by filesystems, so I think it's
> safe to use it to indicate an error. There are also a set of special
> inodes, for instance
>
> #define EXT4_BAD_INO             1      /* Bad blocks inode */
> #define EXT4_ROOT_INO            2      /* Root inode */
> #define EXT4_USR_QUOTA_INO       3      /* User quota inode */
> #define EXT4_GRP_QUOTA_INO       4      /* Group quota inode */
> #define EXT4_BOOT_LOADER_INO     5      /* Boot loader inode */
> #define EXT4_UNDEL_DIR_INO       6      /* Undelete directory inode */
> #define EXT4_RESIZE_INO          7      /* Reserved group descriptors inode */
> #define EXT4_JOURNAL_INO         8      /* Journal inode */
>
> but that's filesystem-specific.
>

I got it.

> > > >
> > > > 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?).
>
> We only have one user of the inode() function in this series, so it's
> indeed hard to know exactly how to design the API. I'd prefer waiting a
> bit for more use cases to appear before deciding on the semantics of the
> equality operator.
>

Yeah, I agree. If we want to add, we should start with defining
equality and it shall be done later, not this patch series.

-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