[libcamera-devel,05/11] Removes references to std::filesystem, which Android 11's toolchain does not support.

17680 diff mbox series

From: Nicholas Roth <nicholas@rothemail.net>

---
 src/android/camera_hal_config.cpp | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Quoting Nicholas Roth via libcamera-devel (2022-10-24 06:55:37)
> From: Nicholas Roth <nicholas@rothemail.net>
> 
> ---
>  src/android/camera_hal_config.cpp | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> index bacfe4b9..c06c993d 100644
> --- a/src/android/camera_hal_config.cpp
> +++ b/src/android/camera_hal_config.cpp
> @@ -6,7 +6,6 @@
>   */
>  #include "camera_hal_config.h"
>  
> -#include <filesystem>
>  #include <stdlib.h>
>  #include <string>
>  
> @@ -160,13 +159,7 @@ CameraHalConfig::CameraHalConfig()
>   */
>  int CameraHalConfig::parseConfigurationFile()
>  {
> -       std::filesystem::path filePath = LIBCAMERA_SYSCONF_DIR;
> -       filePath /= "camera_hal.yaml";
> -       if (!std::filesystem::is_regular_file(filePath)) {
> -               LOG(HALConfig, Debug)
> -                       << "Configuration file: \"" << filePath << "\" not found";
> -               return -ENOENT;
> -       }
> +       std::string filePath = LIBCAMERA_SYSCONF_DIR "/camera_hal.yaml";
>  
>         File file(filePath);

Given the above checks if the file is not found, could / should we
replace that with:

	  if (!file.exists()) {
	  	LOG(HALConfig, Error)
			<< "Configuration file: \"" << filePath << "\" not found";
		return -ENOENT;
	  }
here?

>         if (!file.open(File::OpenModeFlag::ReadOnly)) {
> -- 
> 2.34.1
>

I was kind of surprised to see this actually because right after creating the File object, we then check if it was successfully created and print an error otherwise. If I understand correctly, the std::filesystem-based check was redundant.

Please correct me if I’m wrong though.

Quoting Nicholas Roth (2022-10-24 15:48:28)
> I was kind of surprised to see this actually because right after
> creating the File object, we then check if it was successfully created
> and print an error otherwise. If I understand correctly, the
> std::filesystem-based check was redundant.
> 
> Please correct me if I’m wrong though.

It could be seen as redundant yes. I suspect it was to clearly mark the
difference between the file failing to open, and not being there. But as
"man 2 open" states:

        ENOENT O_CREAT is not set and the named file does not exist.

I expect that we should get a -ENOENT if the file isn't there when we
try to call File.open().

File.open() at [0] will only set O_CREAT if the mode is WriteOnly or
ReadWrite is used, and here we explicitly use ReadOnly at [1].

So I think removing the check entirely is fine and reasonable with a
clearly updated commit message, also explaining the redundancy and that
it's safe to remove without changing the underlying functionality.

Well it might be a very small subtle difference between the
implementation that's removed and returning -ENOENT if it doesn't exist,
as we're perhaps not checking that it's a "regular file" but I don't
think it's too crucial.

Perhaps a note in the commit that we no longer validate it's a regular
file, but expect it to be readable, and otherwise parsed by the
parseConfigFile() to validate the content.



[0] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/file.cpp#n157
[1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_hal_config.cpp#n172

--
Kieran

Hi Nicholas,

On Mon, Oct 24, 2022 at 09:48:28AM -0500, Nicholas Roth via libcamera-devel wrote:
> I was kind of surprised to see this actually because right after
> creating the File object, we then check if it was successfully created
> and print an error otherwise. If I understand correctly, the
> std::filesystem-based check was redundant.
> 
> Please correct me if I’m wrong though.

Checking the file.open() return value will indeed catch this, but that
will result in a different behaviour. The current code reads as

	std::filesystem::path filePath = LIBCAMERA_SYSCONF_DIR;
	filePath /= "camera_hal.yaml";
	if (!std::filesystem::is_regular_file(filePath)) {
		LOG(HALConfig, Debug)
			<< "Configuration file: \"" << filePath << "\" not found";
		return -ENOENT;
	}

	File file(filePath);
	if (!file.open(File::OpenModeFlag::ReadOnly)) {
		int ret = file.error();
		LOG(HALConfig, Error) << "Failed to open configuration file "
				      << filePath << ": " << strerror(-ret);
		return ret;
	}

If the first check fails, the function prints a Debug message and
returns -ENOENT. The HAL configuration file is mandatory, so we don't
want to print an Error message in that case, it would confuse the user.
Then, if the file is found, but we can't open it, an error message is
printed, as that's not a valid situation.

This behaviour could be replicated by using file.exist() as Kieran
proposed, before opening the file object. There would still be a very
small difference in behaviour, as File::exist() will return true for
special files (pipes, character or block devices, ...) while
std::filesystem::is_regular_file() doesn't, but I think that's such a
corner case that it doesn't matter much.

By the way, if that's not too difficult in your e-mail client, could you
avoid removing all context when replying ? It makes reading your e-mails
more difficult otherwise.

Hi Kieran,

On Mon, Oct 24, 2022 at 04:30:05PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Nicholas Roth (2022-10-24 15:48:28)
> > I was kind of surprised to see this actually because right after
> > creating the File object, we then check if it was successfully created
> > and print an error otherwise. If I understand correctly, the
> > std::filesystem-based check was redundant.
> > 
> > Please correct me if I’m wrong though.
> 
> It could be seen as redundant yes. I suspect it was to clearly mark the
> difference between the file failing to open, and not being there. But as
> "man 2 open" states:
> 
>         ENOENT O_CREAT is not set and the named file does not exist.
> 
> I expect that we should get a -ENOENT if the file isn't there when we
> try to call File.open().
> 
> File.open() at [0] will only set O_CREAT if the mode is WriteOnly or
> ReadWrite is used, and here we explicitly use ReadOnly at [1].
> 
> So I think removing the check entirely is fine and reasonable with a
> clearly updated commit message, also explaining the redundancy and that
> it's safe to remove without changing the underlying functionality.
> 
> Well it might be a very small subtle difference between the
> implementation that's removed and returning -ENOENT if it doesn't exist,
> as we're perhaps not checking that it's a "regular file" but I don't
> think it's too crucial.

That's what I've just written in my separate reply to Nicholas' patch
:-)

There's another difference though, a non-existing file will log a Debug
message and a failure to open an Error message. As the HAL configuration
file is not mandatory, we should print an Error message if it's not
found.

> Perhaps a note in the commit that we no longer validate it's a regular
> file, but expect it to be readable, and otherwise parsed by the
> parseConfigFile() to validate the content.
> 
> 
> 
> [0] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/file.cpp#n157
> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_hal_config.cpp#n172