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

[Laurent Pinchart]

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 at web.de>
> > Signed-off-by: Kieran Bingham <kieran.bingham at 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 at 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;

-- 
Regards,

Laurent Pinchart