[libcamera-devel] [PATCH] libcamera: yaml_parser: Use C locale
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 21 20:44:00 CET 2022
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
More information about the libcamera-devel
mailing list