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

[Laurent Pinchart]

Hi Kieran,

On Mon, Jan 11, 2021 at 05:48:20AM +0000, Kieran Bingham wrote:
> On 10/01/2021 11:04, Laurent Pinchart wrote:
> > 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.
> 
> That's why I added it this way. File::exists() expects that you could
> then operate on the target with the other operations of File - which you
> can't if it's a directory.
> 
> >> 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.
> 
> It feels very much like we just duplicate QT all the time. Should we
> just use QT and call it a dependency?

That would be a fairly large dependency, although I'd love to see it
being shipped in all Chrome OS and Android devices ;-)

> At what point do all our helpers simply become 'yet another library'?

We have a bare minimum set of helpers, to accommodate our own needs. We
may consider switching to another helper library instead, such as boost
;-) Jokes aside, I'd love it if there was a small library that offered
all the helpers we need and nothing else.

> >> 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 ?
> 
> I hit an instance where I ensured a file exists before trying to read
> it. (A file, not a directory) - and thus used File::exists() expecting
> it to ... you know, be a file.
> 
> One of the code paths from the ipa proxy passed in a directory, and it
> crashed. Because the directory existed. That seemed flawed, so I sent
> the patch to make File::exists check for files, not directories.

Note that even if a file exists, you may not have permissions to open
it, so we shouldn't expect open() to always succeed. Regardless of that,
the File::exists() API seems fine as it is today.

> We could make a Directory helper, or change it to use isFile()
> isDirectory() ...
> 
> >>>>  		char *ofPath = realpath(node.c_str(), nullptr);
> >>>>  		if (!ofPath)
> >>>>  			return {};

-- 
Regards,

Laurent Pinchart