[libcamera-devel] [PATCH] libcamera: sysfs: Fix directory exists check

[Laurent Pinchart]

Hi Niklas,

On Sat, Jan 09, 2021 at 03:33:43AM +0100, Niklas Söderlund wrote:
> On 2021-01-09 03:38:08 +0200, Laurent Pinchart wrote:
> > On Fri, Jan 08, 2021 at 06:00:42PM +0100, Niklas Söderlund wrote:
> > > The scope of File::exists() was changed to only validate that a file
> > > exists and is therefore not usable to check if a directory exists. This
> > > breaks the persistent name generation for DT based systems as it uses
> > > File::exists() to check for directories, fix this by using stat()
> > > directly.
> > > 
> > > On Scarlet the persistent names of the cameras are impacted by this and
> > > where incorrectly reported as:
> > > 
> > >     $ cam -l
> > >     Available cameras:
> > >     1: Internal front camera (platform/ff160000.i2c/i2c-7/7-003c ov2685)
> > >     2: Internal front camera (platform/ff160000.i2c/i2c-7/7-0036 ov5695
> > > 
> > > While the expected ones are restored with this fix:
> > > 
> > >     $ cam -l
> > >     Available cameras:
> > >     1: Internal front camera (/base/i2c at ff160000/camera at 3c)
> > >     2: Internal front camera (/base/i2c at ff160000/camera at 36)
> > > 
> > > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files")
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/libcamera/sysfs.cpp | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libcamera/sysfs.cpp b/src/libcamera/sysfs.cpp
> > > index 3ebe66f8d69b61d4..e9004b2b59c8638d 100644
> > > --- a/src/libcamera/sysfs.cpp
> > > +++ b/src/libcamera/sysfs.cpp
> > > @@ -70,10 +70,11 @@ std::string charDevPath(const std::string &deviceNode)
> > >  std::string firmwareNodePath(const std::string &device)
> > >  {
> > >  	std::string fwPath, node;
> > > +	struct stat st;
> > >  
> > >  	/* Lookup for DT-based systems */
> > >  	node = device + "/of_node";
> > > -	if (File::exists(node)) {
> > > +	if (!stat(node.c_str(), &st)) {
> > 
> > Hmmm... I think a helper class for this would make sense. I thus wonder
> > if we shouldn't revert 8f4e16f014c820a0.
> 
> I'm starting to lean towards that some helpers are not particular 
> helpful :-) I primarily think our File class adds value as it eases 
> usage of map() and unmap(). Wrapping seek(), read() and write() to 
> better cope with C++ data structures is nice but could perhaps be done 
> as streams if it was just that? I think 8f4e16f014c820a0 adds value as 
> it restricts the File class to only work with files.

It would certainly match the name of the class, and with open()
accepting directories but mmap(), seek(), read() and write() not being
able to operate on them, the new behaviour of File::exists() is
coherent.

> We could of add a Directory::exists() helper to deal with this. But I 
> think that is going down a bad road as I'm sure someone at some point 
> want to just check if a path exists and don't really care if it's a 
> directory or a file (or symlink). Should we create a helper for each 
> type of lookup or have implementations use multiple helpers at the cost 
> of performance? 

A Directory class may be helpful to deal with directories in a better
way than readdir(), but that's not required now. Something along the
lines of https://doc.qt.io/qt-5/qfileinfo.html may also make sense at
some point.

> I'm think it would be cleaner to use primitives of our environment to 
> deal with things that require no context or cleanup outside of the local 
> scope. It's easier to 'man stat' while reading the code then grepping 
> around in the code base for exactly what File::exists() checks for.
> 
> That being said I do not feel strongly about this particular patch it 
> just happened to align with my over all feeling for similar situations.

If you think a FileInfo class would make sense in the future, how about
adding

	 /* \todo Replace the stat call with a FileInfo helper class */

? With or without this,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > Kieran, could you elaborate a little bit on the issue you've encountered
> > that led to that commit being developed ?
> > 
> > >  		char *ofPath = realpath(node.c_str(), nullptr);
> > >  		if (!ofPath)
> > >  			return {};

-- 
Regards,

Laurent Pinchart