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

[Kieran Bingham]

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?

(edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)


> 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...

I guess a FileDescriptor is just a pointer, and the inode is the pointer
value ;-)

> + *
> + * \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.

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 ...


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..


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) {
>