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

[Laurent Pinchart]

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;
> > +
> > +	struct stat st;
> > +	int ret = fstat(fd_->fd(), &st);
> > +	if (ret < 0)
> > +		return 0;
> > +
> > +	return st.st_ino;
> 
> 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.

> 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