[libcamera-devel] [RFC PATCH v1 02/12] libcamera: file_descriptor: Add a function to retrieve the inode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 3 12:34:47 CEST 2021
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'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.
> > > 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
More information about the libcamera-devel
mailing list