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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-01-09 03:38:08 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> 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.

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? 

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.

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

-- 
Regards,
Niklas Söderlund