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

18043 diff mbox series

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.

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@web.de>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/yaml_parser.cpp | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Quoting Kieran Bingham (2022-12-21 17:38:47)
> 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.
> 
> Provide an RAII implementation to construct a locale specific to the
> expected mappings for configuration files provided by libcamera.

We might want to document somewhere that this implies/requires all
configuration files to be written with the 'C' locale ... But I have no
idea where we would document that currently.

--
Kieran


> 
> 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@web.de>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index d8a7c2f9250f..8ddc6d6b5fd5 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -31,6 +31,36 @@ 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.
> + *
> + * For more information see:
> + * - https://bugs.libcamera.org/show_bug.cgi?id=174
> + */
> +class Locale
> +{
> +public:
> +       Locale(std::string locale)
> +       {
> +               locale_ = newlocale(LC_ALL_MASK, locale.c_str(), (locale_t)0);
> +               if (locale_ == (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");
> +
>  } /* namespace */
>  
>  /**
> @@ -283,7 +313,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;
> -- 
> 2.34.1
>

Hi Kieran,

Thank you for the patch.

On Wed, Dec 21, 2022 at 05:44:28PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Kieran Bingham (2022-12-21 17:38:47)
> > 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.
> > 
> > Provide an RAII implementation to construct a locale specific to the
> > expected mappings for configuration files provided by libcamera.

If we ever need to parse non-YAML data with strtod() all of this should
move to the utils namespace, but for now it can live here.

> We might want to document somewhere that this implies/requires all
> configuration files to be written with the 'C' locale ... But I have no
> idea where we would document that currently.

Given that's likely to be the default for the vast majority of
developers (not using the "C" locale, but a locale compatible with it
for LC_NUMERIC), and given that any other assumption will very quickly
be caught, I wouldn't worry too much about documenting it.

> > 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@web.de>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/yaml_parser.cpp | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index d8a7c2f9250f..8ddc6d6b5fd5 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -31,6 +31,36 @@ 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.
> > + *
> > + * For more information see:
> > + * - https://bugs.libcamera.org/show_bug.cgi?id=174
> > + */
> > +class Locale
> > +{
> > +public:
> > +       Locale(std::string locale)

Can the argument be a const char * to avoid constructing a temporary
string ?

> > +       {
> > +               locale_ = newlocale(LC_ALL_MASK, locale.c_str(), (locale_t)0);

static_cast<locale_t>(0)

> > +               if (locale_ == (locale_t)0)

Ditto.

> > +                       LOG(YamlParser, Fatal)
> > +                               << "Failed to construct a locale";
> > +       }

Missing blank line.

> > +       ~Locale()
> > +       {
> > +               freelocale(locale_);
> > +       }

Here too.

Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +       locale_t locale() { return locale_; }
> > +
> > +private:
> > +       locale_t locale_;
> > +};
> > +
> > +static Locale yamlLocale("C");
> > +
> >  } /* namespace */
> >  
> >  /**
> > @@ -283,7 +313,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;