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

[Kieran Bingham]

Hi Laurent,

On 10/01/2021 11:04, Laurent Pinchart wrote:
> 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.

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?

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

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

We could make a Directory helper, or change it to use isFile()
isDirectory() ...


>>>>  		char *ofPath = realpath(node.c_str(), nullptr);
>>>>  		if (!ofPath)
>>>>  			return {};

-- 
Regards
--
Kieran