| Message ID | 20210108170042.2849407-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Commit | 7415c139cd1cab73fdd91785a7816eab51e7a567 |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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@ff160000/camera@3c) > 2: Internal front camera (/base/i2c@ff160000/camera@36) > > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") > Signed-off-by: Niklas Söderlund <niklas.soderlund@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. 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 {};
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@ff160000/camera@3c) > > 2: Internal front camera (/base/i2c@ff160000/camera@36) > > > > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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
Hey Niklas, Thank you for the patch. I have tested your patch on my FriendlyElec NanoPC-T4 development board with the OV13850. *Before* I applied your patch the output looked like this: ``` basti@nanopct4:~$ cam -l Available cameras: 1: Internal front camera (platform/ff110000.i2c/i2c-1/1-0010 ov13850) ``` And *after* applying your patch it looks like this: ``` basti@nanopct4:~$ cam -l Available cameras: 1: Internal front camera (/base/i2c@ff110000/ov13850@10) ``` I also tested this patch on my laptop: *Before*: ``` basti@basti-TUXEDO-Book-XA1510:~$ cam -l Available cameras: 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) ``` *After*: ``` basti@basti-TUXEDO-Book-XA1510:~$ cam -l Available cameras: 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) ``` The output on my laptop looks a little odd, but that is another issue. Greetings Sebastian On 08.01.2021 18:00, 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@ff160000/camera@3c) > 2: Internal front camera (/base/i2c@ff160000/camera@36) > >Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") >Signed-off-by: Niklas Söderlund <niklas.soderlund@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)) { > char *ofPath = realpath(node.c_str(), nullptr); > if (!ofPath) > return {}; >-- >2.30.0 > >_______________________________________________ >libcamera-devel mailing list >libcamera-devel@lists.libcamera.org >https://lists.libcamera.org/listinfo/libcamera-devel
Hi Sebastian, Thanks for testing patch. On 2021-01-09 07:12:23 +0100, Sebastian Fricke wrote: > Hey Niklas, > > Thank you for the patch. > I have tested your patch on my FriendlyElec NanoPC-T4 development board > with the OV13850. > *Before* I applied your patch the output looked like this: > ``` > basti@nanopct4:~$ cam -l > Available cameras: > 1: Internal front camera (platform/ff110000.i2c/i2c-1/1-0010 ov13850) > ``` > And *after* applying your patch it looks like this: > ``` > basti@nanopct4:~$ cam -l > Available cameras: > 1: Internal front camera (/base/i2c@ff110000/ov13850@10) > ``` It looks like the patch solves the persistent naming regression on your platform as well. If you test a camera enumeration before 8f4e16f014c820a0 dose it match the later case (/base/i2c@ff110000/ov13850@10) ? > > I also tested this patch on my laptop: > *Before*: > ``` > basti@basti-TUXEDO-Book-XA1510:~$ cam -l > Available cameras: > 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) > ``` > *After*: > ``` > basti@basti-TUXEDO-Book-XA1510:~$ cam -l > Available cameras: > 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) > ``` > > The output on my laptop looks a little odd, but that is another issue. What is odd about your laptop output? > > Greetings > > Sebastian > > On 08.01.2021 18:00, 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@ff160000/camera@3c) > > 2: Internal front camera (/base/i2c@ff160000/camera@36) > > > > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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)) { > > char *ofPath = realpath(node.c_str(), nullptr); > > if (!ofPath) > > return {}; > > -- > > 2.30.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On 09.01.2021 07:35, Niklas Söderlund wrote: >Hi Sebastian, > >Thanks for testing patch. It is my pleasure :) > >On 2021-01-09 07:12:23 +0100, Sebastian Fricke wrote: >> Hey Niklas, >> >> Thank you for the patch. >> I have tested your patch on my FriendlyElec NanoPC-T4 development board >> with the OV13850. >> *Before* I applied your patch the output looked like this: >> ``` >> basti@nanopct4:~$ cam -l >> Available cameras: >> 1: Internal front camera (platform/ff110000.i2c/i2c-1/1-0010 ov13850) >> ``` >> And *after* applying your patch it looks like this: >> ``` >> basti@nanopct4:~$ cam -l >> Available cameras: >> 1: Internal front camera (/base/i2c@ff110000/ov13850@10) >> ``` > >It looks like the patch solves the persistent naming regression on your >platform as well. If you test a camera enumeration before >8f4e16f014c820a0 dose it match the later case >(/base/i2c@ff110000/ov13850@10) ? Here the output exactly one patch before '8f4e16f014c820a0': basti@nanopct4:~/libcamera$ cam -l Available cameras: 1: Internal front camera (/base/i2c@ff110000/ov13850@10) basti@nanopct4:~/libcamera$ git log commit 0ed053245702d193b9c0ad7d8bda9e975ddfb6ed (HEAD -> tmp) Author: Jacopo Mondi <jacopo@jmondi.org> Date: Wed Dec 23 18:34:34 2020 +0100 android: camera_device: Simplify properties.get() And here the output when I build with '8f4e16f014c820a0': basti@nanopct4:~/libcamera$ cam -l Available cameras: 1: Internal front camera (/base/i2c@ff110000/ov13850@10) basti@nanopct4:~/libcamera$ git log commit 8f4e16f014c820a0ecb85e28453b88c277bc859f (HEAD -> tmp) Author: Kieran Bingham <kieran.bingham@ideasonboard.com> Date: Tue Dec 22 13:50:20 2020 +0000 test: file: Check that directories are not treated as files The commit you mention does only touch a unit test, so it seems to be impossible that this is the patch that you fix. I looked it up and the patch that you actually fix is: 0a823785fad27e1eb9a900e80923bc9bde106de9 "libcamera: file: Check file exist()" Here is the output with '0a823785fad27e1eb9a900e80923bc9bde106de9' applied: basti@nanopct4:~/libcamera$ cam -l Available cameras: 1: Internal front camera (platform/ff110000.i2c/i2c-1/1-0010 ov13850) basti@nanopct4:~/libcamera$ git log commit 0a823785fad27e1eb9a900e80923bc9bde106de9 (HEAD -> tmp) Author: Kieran Bingham <kieran.bingham@ideasonboard.com> Date: Mon Nov 30 23:25:40 2020 +0000 libcamera: file: Check files exist() The patch "8f4e16f014c820a0ecb85e28453b88c277bc859f" comes right before "0a823785fad27e1eb9a900e80923bc9bde106de9". So yes, before '..859f' the output was correct. > >> >> I also tested this patch on my laptop: >> *Before*: >> ``` >> basti@basti-TUXEDO-Book-XA1510:~$ cam -l >> Available cameras: >> 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) >> ``` >> *After*: >> ``` >> basti@basti-TUXEDO-Book-XA1510:~$ cam -l >> Available cameras: >> 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) >> ``` >> >> The output on my laptop looks a little odd, but that is another issue. > >What is odd about your laptop output? I mainly think it looks odd because of the starting backslash and the total different structure. But it is no big issue. > >> >> Greetings >> >> Sebastian >> >> On 08.01.2021 18:00, 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@ff160000/camera@3c) >> > 2: Internal front camera (/base/i2c@ff160000/camera@36) >> > >> > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") >> > Signed-off-by: Niklas Söderlund <niklas.soderlund@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)) { >> > char *ofPath = realpath(node.c_str(), nullptr); >> > if (!ofPath) >> > return {}; >> > -- >> > 2.30.0 >> > >> > _______________________________________________ >> > libcamera-devel mailing list >> > libcamera-devel@lists.libcamera.org >> > https://lists.libcamera.org/listinfo/libcamera-devel > >-- >Regards, >Niklas Söderlund
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@ff160000/camera@3c) > > > 2: Internal front camera (/base/i2c@ff160000/camera@36) > > > > > > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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@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 {};
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@ff160000/camera@3c) >>>> 2: Internal front camera (/base/i2c@ff160000/camera@36) >>>> >>>> Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@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@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 {};
Hi Sebastian, On 09/01/2021 08:22, Sebastian Fricke wrote: >> >>> >>> I also tested this patch on my laptop: >>> *Before*: >>> ``` >>> basti@basti-TUXEDO-Book-XA1510:~$ cam -l >>> Available cameras: >>> 1: External camera 'Chicony USB2.0 Camera: Chicony ' >>> (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) >>> ``` >>> *After*: >>> ``` >>> basti@basti-TUXEDO-Book-XA1510:~$ cam -l >>> Available cameras: >>> 1: External camera 'Chicony USB2.0 Camera: Chicony ' >>> (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685) >>> ``` >>> >>> The output on my laptop looks a little odd, but that is another issue. >> >> What is odd about your laptop output? > > I mainly think it looks odd because of the starting backslash and the > total different structure. But it is no big issue. For UVC cameras based on ACPI firmware paths, I believe that's expected: On mine: 1: External camera 'HP Wide Vision FHD Camera: HP W' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) -- Kieran-- Regards -- Kieran
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@ff160000/camera@3c) > >>>> 2: Internal front camera (/base/i2c@ff160000/camera@36) > >>>> > >>>> Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") > >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@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@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 {};
Hi Niklas, On 08/01/2021 17:00, 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@ff160000/camera@3c) > 2: Internal front camera (/base/i2c@ff160000/camera@36) > > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> This certainly returns the existing behaviour so it's a helpful step forwards. We can consider if we need new helpers later. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > 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)) { > char *ofPath = realpath(node.c_str(), nullptr); > if (!ofPath) > return {}; >
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)) { char *ofPath = realpath(node.c_str(), nullptr); if (!ofPath) return {};
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@ff160000/camera@3c) 2: Internal front camera (/base/i2c@ff160000/camera@36) Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files") Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/sysfs.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)