[{"id":26134,"web_url":"https://patchwork.libcamera.org/comment/26134/","msgid":"<167164466842.51115.8772739191374557644@Monstersaurus>","date":"2022-12-21T17:44:28","subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Use C locale","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2022-12-21 17:38:47)\n> When parsing configuration files on systems with differing locales, the\n> use of strtod can produce different results, or in the worst case - fail\n> to parse expected values.\n> \n> Provide an RAII implementation to construct a locale specific to the\n> expected mappings for configuration files provided by libcamera.\n\nWe might want to document somewhere that this implies/requires all\nconfiguration files to be written with the 'C' locale ... But I have no\nidea where we would document that currently.\n\n--\nKieran\n\n\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=174\n> Bug: https://github.com/raspberrypi/libcamera/issues/29\n> Reported-by: https://github.com/kralo\n> Reported-by: Hannes Winkler \n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/yaml_parser.cpp | 32 +++++++++++++++++++++++++++++++-\n> 1 file changed, 31 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> index d8a7c2f9250f..8ddc6d6b5fd5 100644\n> --- a/src/libcamera/yaml_parser.cpp\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -31,6 +31,36 @@ namespace {\n> /* Empty static YamlObject as a safe result for invalid operations */\n> static const YamlObject empty;\n> \n> +/*\n> + * Construct a global RAII locale for use by all YAML parser instances to\n> + * ensure consistency when parsing configuration files and types regardless of\n> + * the host Locale configuration.\n> + *\n> + * For more information see:\n> + * - https://bugs.libcamera.org/show_bug.cgi?id=174\n> + */\n> +class Locale\n> +{\n> +public:\n> + Locale(std::string locale)\n> + {\n> + locale_ = newlocale(LC_ALL_MASK, locale.c_str(), (locale_t)0);\n> + if (locale_ == (locale_t)0)\n> + LOG(YamlParser, Fatal)\n> + << \"Failed to construct a locale\";\n> + }\n> + ~Locale()\n> + {\n> + freelocale(locale_);\n> + }\n> + locale_t locale() { return locale_; }\n> +\n> +private:\n> + locale_t locale_;\n> +};\n> +\n> +static Locale yamlLocale(\"C\");\n> +\n> } /* namespace */\n> \n> /**\n> @@ -283,7 +313,7 @@ std::optional YamlObject::get() const\n> char *end;\n> \n> errno = 0;\n> - double value = std::strtod(value_.c_str(), &end);\n> + double value = strtod_l(value_.c_str(), &end, yamlLocale.locale());\n> \n> if ('\\0' != *end || errno == ERANGE)\n> return std::nullopt;\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1C2A0C3213\n\tfor ;\n\tWed, 21 Dec 2022 17:44:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D01C8633AC;\n\tWed, 21 Dec 2022 18:44:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CFE861F15\n\tfor ;\n\tWed, 21 Dec 2022 18:44:31 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 17157FB;\n\tWed, 21 Dec 2022 18:44:31 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671644672;\n\tbh=XHQU8uhuDKQgN8sXEG7EzZfZo5PzsKp3UcpWkuefz/M=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=B8fKwHuEBTPXluE9m0t80NJ2XtXTf8FP8UIOaWjOEYhbpEGKvFtwOJjqC8itU0W3b\n\turhkvg+/wfKcra3jOmxITOnJITc/tv3VVy23tvfOHdyukCIdYaEQW73c4bWp6d/P/b\n\tODsdYoLmfgVo5qK4vWCLFzxa9aot9uGYJbuz/YydAhvg5o3fowetev9mnhYDMoBR+q\n\tr6CjmjVBsPmtNA+QdqgWkJ7z7YTe/ytVyzcFKL7tzWxVFy8bnP5lvWS6YedWv+lBTR\n\tSFhrUan/iFB8SV2Wd89Z8AK4a1AGyCcLAODr8vEB/BvfX60Iw9CbSQTjQv77DiHp6Q\n\t3xz4gcRGarIkQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671644671;\n\tbh=XHQU8uhuDKQgN8sXEG7EzZfZo5PzsKp3UcpWkuefz/M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GnHH0Z0GsG+zvtIyIB6mqbdCKdv1XwAndybncaVgHFfuh80cdlHqyDFwvX2SlOUTR\n\tpJL2ta2RzHLfFWq8gSsK6CYc4t+Yk6EuIxYJkCepw2T4Y8XYPEjRx1iTowineBJ6iz\n\tHy2TFraQ3HhgEh4y9P4Vqr/h9OqV8baHoKMOXjvI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GnHH0Z0G\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221221173847.73663-1-kieran.bingham@ideasonboard.com>","References":"<20221221173847.73663-1-kieran.bingham@ideasonboard.com>","To":"libcamera devel ","Date":"Wed, 21 Dec 2022 17:44:28 +0000","Message-ID":"<167164466842.51115.8772739191374557644@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Use C locale","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Hannes Winkler ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26135,"web_url":"https://patchwork.libcamera.org/comment/26135/","msgid":"","date":"2022-12-21T19:44:00","subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Use C locale","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Dec 21, 2022 at 05:44:28PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Kieran Bingham (2022-12-21 17:38:47)\n> > When parsing configuration files on systems with differing locales, the\n> > use of strtod can produce different results, or in the worst case - fail\n> > to parse expected values.\n> > \n> > Provide an RAII implementation to construct a locale specific to the\n> > expected mappings for configuration files provided by libcamera.\n\nIf we ever need to parse non-YAML data with strtod() all of this should\nmove to the utils namespace, but for now it can live here.\n\n> We might want to document somewhere that this implies/requires all\n> configuration files to be written with the 'C' locale ... But I have no\n> idea where we would document that currently.\n\nGiven that's likely to be the default for the vast majority of\ndevelopers (not using the \"C\" locale, but a locale compatible with it\nfor LC_NUMERIC), and given that any other assumption will very quickly\nbe caught, I wouldn't worry too much about documenting it.\n\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=174\n> > Bug: https://github.com/raspberrypi/libcamera/issues/29\n> > Reported-by: https://github.com/kralo\n> > Reported-by: Hannes Winkler \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/libcamera/yaml_parser.cpp | 32 +++++++++++++++++++++++++++++++-\n> > 1 file changed, 31 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> > index d8a7c2f9250f..8ddc6d6b5fd5 100644\n> > --- a/src/libcamera/yaml_parser.cpp\n> > +++ b/src/libcamera/yaml_parser.cpp\n> > @@ -31,6 +31,36 @@ namespace {\n> > /* Empty static YamlObject as a safe result for invalid operations */\n> > static const YamlObject empty;\n> > \n> > +/*\n> > + * Construct a global RAII locale for use by all YAML parser instances to\n> > + * ensure consistency when parsing configuration files and types regardless of\n> > + * the host Locale configuration.\n> > + *\n> > + * For more information see:\n> > + * - https://bugs.libcamera.org/show_bug.cgi?id=174\n> > + */\n> > +class Locale\n> > +{\n> > +public:\n> > + Locale(std::string locale)\n\nCan the argument be a const char * to avoid constructing a temporary\nstring ?\n\n> > + {\n> > + locale_ = newlocale(LC_ALL_MASK, locale.c_str(), (locale_t)0);\n\nstatic_cast(0)\n\n> > + if (locale_ == (locale_t)0)\n\nDitto.\n\n> > + LOG(YamlParser, Fatal)\n> > + << \"Failed to construct a locale\";\n> > + }\n\nMissing blank line.\n\n> > + ~Locale()\n> > + {\n> > + freelocale(locale_);\n> > + }\n\nHere too.\n\nConditionally-Reviewed-by: Laurent Pinchart \n\n> > + locale_t locale() { return locale_; }\n> > +\n> > +private:\n> > + locale_t locale_;\n> > +};\n> > +\n> > +static Locale yamlLocale(\"C\");\n> > +\n> > } /* namespace */\n> > \n> > /**\n> > @@ -283,7 +313,7 @@ std::optional YamlObject::get() const\n> > char *end;\n> > \n> > errno = 0;\n> > - double value = std::strtod(value_.c_str(), &end);\n> > + double value = strtod_l(value_.c_str(), &end, yamlLocale.locale());\n> > \n> > if ('\\0' != *end || errno == ERANGE)\n> > return std::nullopt;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 98FB5C3200\n\tfor ;\n\tWed, 21 Dec 2022 19:44:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEC1E633AC;\n\tWed, 21 Dec 2022 20:44:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69A0A61F15\n\tfor ;\n\tWed, 21 Dec 2022 20:44:06 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B78DDFB;\n\tWed, 21 Dec 2022 20:44:05 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671651848;\n\tbh=nip12PZ5kInLnUjzp/kcgsn6pVyitkOX+AbXg2o0wF0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=anRp3upcCohOs0hchwV4pf95Jez0WuLQyhlU2scZNtHh6N/9SIZF3F8tHhfJUKLIo\n\t67V0WTF0R3AArQAVDBbftGgq1FVhRUj0Hrc5JuAtu723ZJabSJ/B2UrfKY325mcf9Z\n\tyQwVQd37cvoEzpO/njCnPYWJS5hopthU8lekKuMRp7hXXaammAl5q4/66Ugc8nVn1h\n\tOlstCN5E0TUZXBCe4A6ohciP0z1d59qHYDN2eMbJQU1Jqjs/npT7N/ZnGg875+6kFM\n\tKAh++b1PU+M8tSyDOEO7//sg56G6bJgKJh8y+XauG1Bme+mK1qxm9srAz64vxJvXsk\n\tPuQ9VcctEQz7g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671651845;\n\tbh=nip12PZ5kInLnUjzp/kcgsn6pVyitkOX+AbXg2o0wF0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LdW5cKtxABDPi8ioMFvgCVM2ebnc5KlsT7qTnwKsu0qKJCkDVkX7ONZExLoiyyYEI\n\tEAg9eKD5JswHE7QSJdcKFO4sWO5Wx/ovc28RVqLOby1Gor6FRkArLLygjpnde4FLI1\n\tulCcPn39uZy2ywO3Ah/PqLj8nL+OFUQ5bFq2xEBA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LdW5cKtx\"; dkim-atps=neutral","Date":"Wed, 21 Dec 2022 21:44:00 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221221173847.73663-1-kieran.bingham@ideasonboard.com>\n\t<167164466842.51115.8772739191374557644@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<167164466842.51115.8772739191374557644@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Use C locale","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ,\n\tHannes Winkler ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]