[libcamera-devel] [PATCH v2] libcamera: yaml_parser: Use C locale

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Dec 30, 2022 at 12:23:09AM +0000, Kieran Bingham wrote:
> When parsing configuration files on systems with differing locales, the
> use of strtod can produce different results, or in the worst case - fail
> to parse expected values.

There's something missing here:

Fix this by using strtod_l() instead. To avoid constructing and
destructing a locale_t instance for every too to strtod_l(), create an
RAII class that wraps the locale_t and use it to provide a global "C"
locale.

> Provide an RAII implementation to construct a locale specific to the
> expected mappings for configuration files provided by libcamera.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=174
> Bug: https://github.com/raspberrypi/libcamera/issues/29
> Reported-by: https://github.com/kralo
> Reported-by: Hannes Winkler <hanneswinkler2000 at web.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> v2:
>  - use const char * over std::string
>  - use C++ static casts
>  - fix blank lines
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index d8a7c2f9250f..c7fd80cdea1b 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -31,6 +31,38 @@ namespace {
>  /* Empty static YamlObject as a safe result for invalid operations */
>  static const YamlObject empty;
>  
> +/*
> + * Construct a global RAII locale for use by all YAML parser instances to
> + * ensure consistency when parsing configuration files and types regardless of
> + * the host Locale configuration.

s/host/system/
s/Locale/locale/

> + *
> + * For more information see:
> + * - https://bugs.libcamera.org/show_bug.cgi?id=174
> + */
> +class Locale
> +{
> +public:
> +	Locale(const char *locale)
> +	{
> +		locale_ = newlocale(LC_ALL_MASK, locale, static_cast<locale_t>(0));
> +		if (locale_ == static_cast<locale_t>(0))
> +			LOG(YamlParser, Fatal)
> +				<< "Failed to construct a locale";
> +	}
> +
> +	~Locale()
> +	{
> +		freelocale(locale_);
> +	}
> +
> +	locale_t locale() { return locale_; }
> +
> +private:
> +	locale_t locale_;
> +};
> +
> +static Locale yamlLocale("C");

You can drop static as you're in an anonymous namespace.

All these issues are minor, no need to post a v3 just for this.

> +
>  } /* namespace */
>  
>  /**
> @@ -283,7 +315,7 @@ std::optional<double> YamlObject::get() const
>  	char *end;
>  
>  	errno = 0;
> -	double value = std::strtod(value_.c_str(), &end);
> +	double value = strtod_l(value_.c_str(), &end, yamlLocale.locale());
>  
>  	if ('\0' != *end || errno == ERANGE)
>  		return std::nullopt;

-- 
Regards,

Laurent Pinchart