[libcamera-devel,v1,2/5] v4l2: v4l2_camera_file: Store file description
diff mbox series

Message ID 20211228215951.32396-3-laurent.pinchart@ideasonboard.com
State Accepted
Delegated to: Laurent Pinchart
Headers show
Series
  • libcamera: Improve debug messages in V4L2 compat layer
Related show

Commit Message

Laurent Pinchart Dec. 28, 2021, 9:59 p.m. UTC 
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@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(-)

Comments

Kieran Bingham Jan. 10, 2022, 4:41 p.m. UTC | #1 
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@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.

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

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@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
>
Laurent Pinchart Jan. 10, 2022, 4:52 p.m. UTC | #2 
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@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@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;
Paul Elder Jan. 14, 2022, 10:40 a.m. UTC | #3 
Hi Laurent,

On Tue, Dec 28, 2021 at 11:59:48PM +0200, Laurent Pinchart wrote:
> 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@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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)/");
> +			}
> +		} 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
>

Patch
diff mbox series 

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)/");
+			}
+		} 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;