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

[Hirokazu Honda]

Hi Laurent,

On Mon, Sep 6, 2021 at 8:41 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
>

Ah, I didn't notice it. Thanks for explaining.

-Hiro

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