[libcamera-devel] [PATCH] libcamera: sysfs: Fix directory exists check
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jan 11 06:48:20 CET 2021
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
More information about the libcamera-devel
mailing list