[libcamera-devel] [PATCH v2 02/27] libcamera: file_descriptor: Add a function to retrieve the inode

[Laurent Pinchart]

Hi Hiro,

On Mon, Sep 06, 2021 at 08:37:26PM +0900, Hirokazu Honda wrote:
> On Mon, Sep 6, 2021 at 6:04 PM <paul.elder at ideasonboard.com> wrote:
> > On Mon, Sep 06, 2021 at 05:00:35AM +0300, Laurent Pinchart wrote:
> > > The inode is useful to check if two file descriptors refer to the same
> > > file. Add a function to retrieve it.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > > ---
> > > Changes since v1:
> > >
> > > - Use isValid() instead of open-coding it
> > > - Print a message on error
> > > ---
> > >  include/libcamera/file_descriptor.h |  3 +++
> > >  src/libcamera/file_descriptor.cpp   | 26 ++++++++++++++++++++++++++
> > >  2 files changed, 29 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..0409c3e1758c 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,30 @@ FileDescriptor FileDescriptor::dup() const
> > >       return FileDescriptor(fd());
> > >  }
> > >
> > > +/**
> > > + * \brief Retrieve the file descriptor inode
> > > + *
> > > + * \todo Should this move to the File class ?
> > > + *
> > > + * \return The file descriptor inode on success, or 0 on error
> > > + */
> > > +ino_t FileDescriptor::inode() const
> > > +{
> > > +     if (!isValid())
> > > +             return 0;
> > > +
> > > +     struct stat st;
> > > +     int ret = fstat(fd_->fd(), &st);
> > > +     if (ret < 0) {
> > > +             ret = -errno;
> > > +             LOG(FileDescriptor, Fatal)
> > > +                     << "Failed to fstat() fd: " << strerror(-ret);
> 
> Setting errno to ret is unnecessary?
> 
> if (ret < 0) {
>    LOG(FileDescriptor, Fatal) << "Failed to fstat() fd: " << stderr(errno);
>   return 0;
> }

The LOG() macros constructs a LogMessage object, and it may change errno
before strerror() is called. The same is possibly true of the
operator<<() calls, or other function calls in the log line. Maybe it
can't happen in this specific case, but we always assign ret = -errno to
be safe.

> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> 
> > > +             return 0;
> > > +     }
> > > +
> > > +     return st.st_ino;
> > > +}
> > > +
> > >  FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
> > >  {
> > >       if (!duplicate) {

-- 
Regards,

Laurent Pinchart