[libcamera-devel] [PATCH v1 2/5] v4l2: v4l2_camera_file: Store file description

[Laurent Pinchart]

Hi Kieran,

On Mon, Jan 10, 2022 at 04:41:08PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-12-28 21:59:48)
> > Create a string that describe the file from the path and file
> > descriptor. This will be used in log messages to clearly identify which
> > file an operation is related to.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_camera_file.cpp    | 28 +++++++++++++++++++++++++++-
> >  src/v4l2/v4l2_camera_file.h      |  8 +++++++-
> >  src/v4l2/v4l2_compat_manager.cpp |  4 +++-
> >  3 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/v4l2/v4l2_camera_file.cpp b/src/v4l2/v4l2_camera_file.cpp
> > index a07679b587ce..0a41587ca90b 100644
> > --- a/src/v4l2/v4l2_camera_file.cpp
> > +++ b/src/v4l2/v4l2_camera_file.cpp
> > @@ -7,20 +7,46 @@
> >  
> >  #include "v4l2_camera_file.h"
> >  
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +
> >  #include <linux/videodev2.h>
> >  
> >  #include "v4l2_camera_proxy.h"
> >  
> >  using namespace libcamera;
> >  
> > -V4L2CameraFile::V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy)
> > +V4L2CameraFile::V4L2CameraFile(int dirfd, const char *path, int efd,
> > +                              bool nonBlocking, V4L2CameraProxy *proxy)
> >         : proxy_(proxy), nonBlocking_(nonBlocking), efd_(efd),
> >           priority_(V4L2_PRIORITY_DEFAULT)
> >  {
> >         proxy_->open(this);
> > +
> > +       if (path[0] != '/') {
> > +               if (dirfd == AT_FDCWD) {
> > +                       char *cwd = getcwd(nullptr, 0);
> > +                       if (cwd) {
> > +                               description_ = std::string(cwd) + "/";
> > +                               free(cwd);
> > +                       } else {
> > +                               description_ = std::string("(unreachable)/");
> 
> I would have thought this could have been './' but I don't expect it to
> fail in these circumstances. Though saying that, man getcwd() does state
> that the nullptr,0 parameters are a glibc specific extension - so we
> could end up with this failing in fact.

I've checked uclibc, musl and bionic, and they all implement the
extension, so I think we're good.

> So (unreachable)/ is probably fine, but we could also at that point
> allocate our own memory to actaully obtain it... (or .. use '.').

The "(unreachable)" name, by the way, comes from Linux 2.6.36 that uses
that prefix when the current directory is not below the root directory
of the current process. In any case, I don't ever expect users to see
this.

> Anyway, I don't think that's too important here, as it's probably an
> edge case (until someone tests otherwise).
> 
> So,
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +                       }
> > +               } else {
> > +                       description_ = "(dirfd:" + std::to_string(dirfd) + ")/";
> > +               }
> > +       }
> > +
> > +       description_ += std::string(path) + " (fd:" + std::to_string(efd) + ")";
> >  }
> >  
> >  V4L2CameraFile::~V4L2CameraFile()
> >  {
> >         proxy_->close(this);
> >  }
> > +
> > +const std::string &V4L2CameraFile::description() const
> > +{
> > +       return description_;
> > +}
> > diff --git a/src/v4l2/v4l2_camera_file.h b/src/v4l2/v4l2_camera_file.h
> > index 6c4cb5d89dbf..1a7b6a63ae95 100644
> > --- a/src/v4l2/v4l2_camera_file.h
> > +++ b/src/v4l2/v4l2_camera_file.h
> > @@ -7,6 +7,8 @@
> >  
> >  #pragma once
> >  
> > +#include <string>
> > +
> >  #include <linux/videodev2.h>
> >  
> >  class V4L2CameraProxy;
> > @@ -14,7 +16,8 @@ class V4L2CameraProxy;
> >  class V4L2CameraFile
> >  {
> >  public:
> > -       V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy);
> > +       V4L2CameraFile(int dirfd, const char *path, int efd, bool nonBlocking,
> > +                      V4L2CameraProxy *proxy);
> >         ~V4L2CameraFile();
> >  
> >         V4L2CameraProxy *proxy() const { return proxy_; }
> > @@ -25,9 +28,12 @@ public:
> >         enum v4l2_priority priority() const { return priority_; }
> >         void setPriority(enum v4l2_priority priority) { priority_ = priority; }
> >  
> > +       const std::string &description() const;
> > +
> >  private:
> >         V4L2CameraProxy *proxy_;
> >  
> > +       std::string description_;
> >         bool nonBlocking_;
> >         int efd_;
> >         enum v4l2_priority priority_;
> > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > index c64cdb862489..585046e97e4b 100644
> > --- a/src/v4l2/v4l2_compat_manager.cpp
> > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > @@ -156,7 +156,9 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod
> >                 return efd;
> >  
> >         V4L2CameraProxy *proxy = proxies_[ret].get();
> > -       files_.emplace(efd, std::make_shared<V4L2CameraFile>(efd, oflag & O_NONBLOCK, proxy));
> > +       files_.emplace(efd, std::make_shared<V4L2CameraFile>(dirfd, path, efd,
> > +                                                            oflag & O_NONBLOCK,
> > +                                                            proxy));
> >  
> >         LOG(V4L2Compat, Debug) << "Opened " << path << " -> fd " << efd;
> >         return efd;

-- 
Regards,

Laurent Pinchart