[{"id":24638,"web_url":"https://patchwork.libcamera.org/comment/24638/","msgid":"<20220818130843.GI2412817@pyrite.rasen.tech>","date":"2022-08-18T13:08:43","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser:\n\tDe-duplicate common code in YamlObject::get()","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Tue, Aug 16, 2022 at 04:54:11AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The specializations of the YamlObject::get() function template for\n> integer types duplicate code that doesn't directly depend on the\n> template type argument. Move it to separate helper functions to reduce\n> the object size.\n> \n> While at it, rephrase the comment about unsigned integer parsing.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/yaml_parser.cpp | 160 ++++++++++++++--------------------\n> 1 file changed, 67 insertions(+), 93 deletions(-)\n> \n> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> index 85a52c05e682..3d5efdc4419b 100644\n> --- a/src/libcamera/yaml_parser.cpp\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -131,23 +131,65 @@ std::optional YamlObject::get() const\n> \treturn std::nullopt;\n> }\n> \n> +namespace {\n> +\n> +bool parseSignedInteger(const std::string &str, int32_t min, int32_t max,\n> +\t\t\tint32_t *result)\n> +{\n> +\tif (str == \"\")\n> +\t\treturn false;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n> +\tlong value = std::strtol(str.c_str(), &end, 10);\n> +\n> +\tif ('\\0' != *end || errno == ERANGE || value < min || value > max)\n> +\t\treturn false;\n> +\n> +\t*result = value;\n> +\treturn true;\n> +}\n> +\n> +bool parseUnsignedInteger(const std::string &str, uint32_t max, uint32_t *result)\n> +{\n> +\tif (str == \"\")\n> +\t\treturn false;\n> +\n> +\t/*\n> +\t * strtoul() accepts strings representing a negative number, in which\n> +\t * case it negates the converted value. We don't want to silently accept\n> +\t * negative values and return a large positive number, so check for a\n> +\t * minus sign (after optional whitespace) and return an error.\n> +\t */\n> +\tstd::size_t found = str.find_first_not_of(\" \\t\");\n> +\tif (found != std::string::npos && str[found] == '-')\n> +\t\treturn false;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n> +\tunsigned long value = std::strtoul(str.c_str(), &end, 10);\n> +\n> +\tif ('\\0' != *end || errno == ERANGE || value > max)\n> +\t\treturn false;\n> +\n> +\t*result = value;\n> +\treturn true;\n> +}\n> +\n> +} /* namespace */\n> +\n> template<>\n> std::optional YamlObject::get() const\n> {\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n> \n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tint32_t value;\n> \n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> +\t\t\t\tstd::numeric_limits::max(), &value))\n> \t\treturn std::nullopt;\n> \n> \treturn value;\n> @@ -159,28 +201,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n> \n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tuint32_t value;\n> \n> -\t/*\n> -\t * libyaml parses all scalar values as strings. When a string has\n> -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> -\t * skips leading spaces, accepts the leading minus sign, and the\n> -\t * calculated digits are negated as if by unary minus. Rule it out in\n> -\t * case the user gets a large number when the value is negative.\n> -\t */\n> -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> -\tif (found != std::string::npos && value_[found] == '-')\n> -\t\treturn std::nullopt;\n> -\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> +\t\t\t\t &value))\n> \t\treturn std::nullopt;\n> \n> \treturn value;\n> @@ -192,17 +216,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n> \n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tint32_t value;\n> \n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> +\t\t\t\tstd::numeric_limits::max(), &value))\n> \t\treturn std::nullopt;\n> \n> \treturn value;\n> @@ -214,28 +231,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n> \n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tuint32_t value;\n> \n> -\t/*\n> -\t * libyaml parses all scalar values as strings. When a string has\n> -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> -\t * skips leading spaces, accepts the leading minus sign, and the\n> -\t * calculated digits are negated as if by unary minus. Rule it out in\n> -\t * case the user gets a large number when the value is negative.\n> -\t */\n> -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> -\tif (found != std::string::npos && value_[found] == '-')\n> -\t\treturn std::nullopt;\n> -\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> +\t\t\t\t &value))\n> \t\treturn std::nullopt;\n> \n> \treturn value;\n> @@ -247,17 +246,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n> \n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tint32_t value;\n> \n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> +\t\t\t\tstd::numeric_limits::max(), &value))\n> \t\treturn std::nullopt;\n> \n> \treturn value;\n> @@ -269,28 +261,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n> \n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tuint32_t value;\n> \n> -\t/*\n> -\t * libyaml parses all scalar values as strings. When a string has\n> -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> -\t * skips leading spaces, accepts the leading minus sign, and the\n> -\t * calculated digits are negated as if by unary minus. Rule it out in\n> -\t * case the user gets a large number when the value is negative.\n> -\t */\n> -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> -\tif (found != std::string::npos && value_[found] == '-')\n> -\t\treturn std::nullopt;\n> -\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> +\t\t\t\t &value))\n> \t\treturn std::nullopt;\n> \n> \treturn value;\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 410D5BE173\n\tfor ;\n\tThu, 18 Aug 2022 13:08:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68BF061FC0;\n\tThu, 18 Aug 2022 15:08:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2601E61FA7\n\tfor ;\n\tThu, 18 Aug 2022 15:08:53 +0200 (CEST)","from pyrite.rasen.tech (KD027085204050.au-net.ne.jp [27.85.204.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7EFAA8B;\n\tThu, 18 Aug 2022 15:08:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660828134;\n\tbh=u7/DP3Zc0AA/rlE5euIICPq28AuUTxI6ph23R4Wwlp8=;\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=lDZe0QH7+NP9tqCwZbRdqUJb/nEcFOOamnun1Wx5I6pfylSCU4xJB6of2m/ui8cCc\n\tJtfcJIGj6tk79/ojWblXVOocW/CW9T5qwENhUzpAXdtkWf2UTuFwDcF3vsZNxywZXr\n\tNXT16/LElXv1w8gkN3N+dwbFqSUH+Qe3QdHuchKZAGNiR41r5ejzzijXSXoj7Xze+a\n\tFGGYJX7owKTcHI9Y4ZfV9FtHeAtSvN+ilRwUVQl0HYMuO8JbI/WJSpL8Sl/70hKhlm\n\tOeE5NuxtRYroyWBB6WOtTZaOvRPbO5BzDIs5gqm+9/h7Y/IfZLntiL19v1RjwqtRFS\n\ttCjgh0OownJkQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660828132;\n\tbh=u7/DP3Zc0AA/rlE5euIICPq28AuUTxI6ph23R4Wwlp8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EIgRmzLc8F5Z1y0V5eNioblhAKWgOuy7QklEe/Sw004QmU7Aq3CyjY/4zTooQe3pH\n\tigrospNkRfYJ40xsf59BXWVddSqZ16DsuuHTtx4Jv04b9xCTY75yRXIRDrNQpXfi+D\n\t9l9gm6zbfwix3XXSeWz1xlT+Jua6vWrIhSWIhy7A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EIgRmzLc\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 22:08:43 +0900","To":"Laurent Pinchart ","Message-ID":"<20220818130843.GI2412817@pyrite.rasen.tech>","References":"<20220816015414.7462-1-laurent.pinchart@ideasonboard.com>\n\t<20220816015414.7462-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220816015414.7462-7-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser:\n\tDe-duplicate common code in YamlObject::get()","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24644,"web_url":"https://patchwork.libcamera.org/comment/24644/","msgid":"<20220818134434.wg7bq7o5k7gg3dt6@uno.localdomain>","date":"2022-08-18T13:44:34","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser:\n\tDe-duplicate common code in YamlObject::get()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Aug 16, 2022 at 04:54:11AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The specializations of the YamlObject::get() function template for\n> integer types duplicate code that doesn't directly depend on the\n> template type argument. Move it to separate helper functions to reduce\n> the object size.\n>\n> While at it, rephrase the comment about unsigned integer parsing.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/yaml_parser.cpp | 160 ++++++++++++++--------------------\n> 1 file changed, 67 insertions(+), 93 deletions(-)\n>\n> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> index 85a52c05e682..3d5efdc4419b 100644\n> --- a/src/libcamera/yaml_parser.cpp\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -131,23 +131,65 @@ std::optional YamlObject::get() const\n> \treturn std::nullopt;\n> }\n>\n> +namespace {\n> +\n> +bool parseSignedInteger(const std::string &str, int32_t min, int32_t max,\n> +\t\t\tint32_t *result)\n\nThe usage of int32_t types might not scale if we want to add support\nfor parsing 64-bit values.\n\nFor now I think it's fine\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +{\n> +\tif (str == \"\")\n> +\t\treturn false;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n> +\tlong value = std::strtol(str.c_str(), &end, 10);\n> +\n> +\tif ('\\0' != *end || errno == ERANGE || value < min || value > max)\n> +\t\treturn false;\n> +\n> +\t*result = value;\n> +\treturn true;\n> +}\n> +\n> +bool parseUnsignedInteger(const std::string &str, uint32_t max, uint32_t *result)\n> +{\n> +\tif (str == \"\")\n> +\t\treturn false;\n> +\n> +\t/*\n> +\t * strtoul() accepts strings representing a negative number, in which\n> +\t * case it negates the converted value. We don't want to silently accept\n> +\t * negative values and return a large positive number, so check for a\n> +\t * minus sign (after optional whitespace) and return an error.\n> +\t */\n> +\tstd::size_t found = str.find_first_not_of(\" \\t\");\n> +\tif (found != std::string::npos && str[found] == '-')\n> +\t\treturn false;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n> +\tunsigned long value = std::strtoul(str.c_str(), &end, 10);\n> +\n> +\tif ('\\0' != *end || errno == ERANGE || value > max)\n> +\t\treturn false;\n> +\n> +\t*result = value;\n> +\treturn true;\n> +}\n> +\n> +} /* namespace */\n> +\n> template<>\n> std::optional YamlObject::get() const\n> {\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n>\n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tint32_t value;\n>\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> +\t\t\t\tstd::numeric_limits::max(), &value))\n> \t\treturn std::nullopt;\n>\n> \treturn value;\n> @@ -159,28 +201,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n>\n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tuint32_t value;\n>\n> -\t/*\n> -\t * libyaml parses all scalar values as strings. When a string has\n> -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> -\t * skips leading spaces, accepts the leading minus sign, and the\n> -\t * calculated digits are negated as if by unary minus. Rule it out in\n> -\t * case the user gets a large number when the value is negative.\n> -\t */\n> -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> -\tif (found != std::string::npos && value_[found] == '-')\n> -\t\treturn std::nullopt;\n> -\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> +\t\t\t\t &value))\n> \t\treturn std::nullopt;\n>\n> \treturn value;\n> @@ -192,17 +216,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n>\n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tint32_t value;\n>\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> +\t\t\t\tstd::numeric_limits::max(), &value))\n> \t\treturn std::nullopt;\n>\n> \treturn value;\n> @@ -214,28 +231,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n>\n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tuint32_t value;\n>\n> -\t/*\n> -\t * libyaml parses all scalar values as strings. When a string has\n> -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> -\t * skips leading spaces, accepts the leading minus sign, and the\n> -\t * calculated digits are negated as if by unary minus. Rule it out in\n> -\t * case the user gets a large number when the value is negative.\n> -\t */\n> -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> -\tif (found != std::string::npos && value_[found] == '-')\n> -\t\treturn std::nullopt;\n> -\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> +\t\t\t\t &value))\n> \t\treturn std::nullopt;\n>\n> \treturn value;\n> @@ -247,17 +246,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n>\n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tint32_t value;\n>\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> +\t\t\t\tstd::numeric_limits::max(), &value))\n> \t\treturn std::nullopt;\n>\n> \treturn value;\n> @@ -269,28 +261,10 @@ std::optional YamlObject::get() const\n> \tif (type_ != Type::Value)\n> \t\treturn std::nullopt;\n>\n> -\tif (value_ == \"\")\n> -\t\treturn std::nullopt;\n> +\tuint32_t value;\n>\n> -\t/*\n> -\t * libyaml parses all scalar values as strings. When a string has\n> -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> -\t * skips leading spaces, accepts the leading minus sign, and the\n> -\t * calculated digits are negated as if by unary minus. Rule it out in\n> -\t * case the user gets a large number when the value is negative.\n> -\t */\n> -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> -\tif (found != std::string::npos && value_[found] == '-')\n> -\t\treturn std::nullopt;\n> -\n> -\tchar *end;\n> -\n> -\terrno = 0;\n> -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> -\n> -\tif ('\\0' != *end || errno == ERANGE ||\n> -\t value < std::numeric_limits::min() ||\n> -\t value > std::numeric_limits::max())\n> +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> +\t\t\t\t &value))\n> \t\treturn std::nullopt;\n>\n> \treturn value;\n> --\n> Regards,\n>\n> Laurent Pinchart\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 99948BE173\n\tfor ;\n\tThu, 18 Aug 2022 13:44:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCDDF61FC0;\n\tThu, 18 Aug 2022 15:44:38 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 928CD61FA7\n\tfor ;\n\tThu, 18 Aug 2022 15:44:37 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id D784BFF804;\n\tThu, 18 Aug 2022 13:44:36 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660830278;\n\tbh=ElrvU6vqiw33/Y2AtuNmRO+++tfc3wcxtEQoP2jOFUA=;\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=Sk3IhWRwLjhRIqyuaprWq2353t/V/6tN4de6E1sHXTaNBofYOsD2PtkcVYPMpf/8P\n\tmdfJ9gfL5dOe4Lxmu75fqdpLiafK4IpH7y31DoGE5pKt3uMTBymqejjzW1T2qpnt81\n\tofC7UhsuFkAqzi0fvdCA7brDVPEZzwfCC1nBZcg8dIsN2pV4A8KpeJMNez7YYInwUC\n\tINEG1rf4m9fVP/bZBKhaRNOX2zzuvTsaV7TQXMd8btmBa6PuPEClEL5xCXQrO8Jpm0\n\toUkOvrEcLyspXm06gluR9pfnAWG7uA1DGWWzvBnn0MFvgJM/w8lMm3LdUQOfduv11J\n\tA9kvVLY5aA34w==","Date":"Thu, 18 Aug 2022 15:44:34 +0200","To":"Laurent Pinchart ","Message-ID":"<20220818134434.wg7bq7o5k7gg3dt6@uno.localdomain>","References":"<20220816015414.7462-1-laurent.pinchart@ideasonboard.com>\n\t<20220816015414.7462-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220816015414.7462-7-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser:\n\tDe-duplicate common code in YamlObject::get()","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24677,"web_url":"https://patchwork.libcamera.org/comment/24677/","msgid":"","date":"2022-08-18T17:41:25","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser:\n\tDe-duplicate common code in YamlObject::get()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Aug 18, 2022 at 03:44:34PM +0200, Jacopo Mondi wrote:\n> On Tue, Aug 16, 2022 at 04:54:11AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > The specializations of the YamlObject::get() function template for\n> > integer types duplicate code that doesn't directly depend on the\n> > template type argument. Move it to separate helper functions to reduce\n> > the object size.\n> >\n> > While at it, rephrase the comment about unsigned integer parsing.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/yaml_parser.cpp | 160 ++++++++++++++--------------------\n> > 1 file changed, 67 insertions(+), 93 deletions(-)\n> >\n> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> > index 85a52c05e682..3d5efdc4419b 100644\n> > --- a/src/libcamera/yaml_parser.cpp\n> > +++ b/src/libcamera/yaml_parser.cpp\n> > @@ -131,23 +131,65 @@ std::optional YamlObject::get() const\n> > \treturn std::nullopt;\n> > }\n> >\n> > +namespace {\n> > +\n> > +bool parseSignedInteger(const std::string &str, int32_t min, int32_t max,\n> > +\t\t\tint32_t *result)\n> \n> The usage of int32_t types might not scale if we want to add support\n> for parsing 64-bit values.\n\nI thought about it and then decided to leave it for later, but I think I\ncan already change this to use a long (and an unsigned long below) as\nthose are the return types of strtol and strtoul. It won't give us\n64-bit support on 32-bit platforms, that will need to be handled when\nadding 64-bit integer support in any case.\n\n> For now I think it's fine\n> Reviewed-by: Jacopo Mondi \n> \n> > +{\n> > +\tif (str == \"\")\n> > +\t\treturn false;\n> > +\n> > +\tchar *end;\n> > +\n> > +\terrno = 0;\n> > +\tlong value = std::strtol(str.c_str(), &end, 10);\n> > +\n> > +\tif ('\\0' != *end || errno == ERANGE || value < min || value > max)\n> > +\t\treturn false;\n> > +\n> > +\t*result = value;\n> > +\treturn true;\n> > +}\n> > +\n> > +bool parseUnsignedInteger(const std::string &str, uint32_t max, uint32_t *result)\n> > +{\n> > +\tif (str == \"\")\n> > +\t\treturn false;\n> > +\n> > +\t/*\n> > +\t * strtoul() accepts strings representing a negative number, in which\n> > +\t * case it negates the converted value. We don't want to silently accept\n> > +\t * negative values and return a large positive number, so check for a\n> > +\t * minus sign (after optional whitespace) and return an error.\n> > +\t */\n> > +\tstd::size_t found = str.find_first_not_of(\" \\t\");\n> > +\tif (found != std::string::npos && str[found] == '-')\n> > +\t\treturn false;\n> > +\n> > +\tchar *end;\n> > +\n> > +\terrno = 0;\n> > +\tunsigned long value = std::strtoul(str.c_str(), &end, 10);\n> > +\n> > +\tif ('\\0' != *end || errno == ERANGE || value > max)\n> > +\t\treturn false;\n> > +\n> > +\t*result = value;\n> > +\treturn true;\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > template<>\n> > std::optional YamlObject::get() const\n> > {\n> > \tif (type_ != Type::Value)\n> > \t\treturn std::nullopt;\n> >\n> > -\tif (value_ == \"\")\n> > -\t\treturn std::nullopt;\n> > +\tint32_t value;\n> >\n> > -\tchar *end;\n> > -\n> > -\terrno = 0;\n> > -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> > -\n> > -\tif ('\\0' != *end || errno == ERANGE ||\n> > -\t value < std::numeric_limits::min() ||\n> > -\t value > std::numeric_limits::max())\n> > +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> > +\t\t\t\tstd::numeric_limits::max(), &value))\n> > \t\treturn std::nullopt;\n> >\n> > \treturn value;\n> > @@ -159,28 +201,10 @@ std::optional YamlObject::get() const\n> > \tif (type_ != Type::Value)\n> > \t\treturn std::nullopt;\n> >\n> > -\tif (value_ == \"\")\n> > -\t\treturn std::nullopt;\n> > +\tuint32_t value;\n> >\n> > -\t/*\n> > -\t * libyaml parses all scalar values as strings. When a string has\n> > -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> > -\t * skips leading spaces, accepts the leading minus sign, and the\n> > -\t * calculated digits are negated as if by unary minus. Rule it out in\n> > -\t * case the user gets a large number when the value is negative.\n> > -\t */\n> > -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> > -\tif (found != std::string::npos && value_[found] == '-')\n> > -\t\treturn std::nullopt;\n> > -\n> > -\tchar *end;\n> > -\n> > -\terrno = 0;\n> > -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> > -\n> > -\tif ('\\0' != *end || errno == ERANGE ||\n> > -\t value < std::numeric_limits::min() ||\n> > -\t value > std::numeric_limits::max())\n> > +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> > +\t\t\t\t &value))\n> > \t\treturn std::nullopt;\n> >\n> > \treturn value;\n> > @@ -192,17 +216,10 @@ std::optional YamlObject::get() const\n> > \tif (type_ != Type::Value)\n> > \t\treturn std::nullopt;\n> >\n> > -\tif (value_ == \"\")\n> > -\t\treturn std::nullopt;\n> > +\tint32_t value;\n> >\n> > -\tchar *end;\n> > -\n> > -\terrno = 0;\n> > -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> > -\n> > -\tif ('\\0' != *end || errno == ERANGE ||\n> > -\t value < std::numeric_limits::min() ||\n> > -\t value > std::numeric_limits::max())\n> > +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> > +\t\t\t\tstd::numeric_limits::max(), &value))\n> > \t\treturn std::nullopt;\n> >\n> > \treturn value;\n> > @@ -214,28 +231,10 @@ std::optional YamlObject::get() const\n> > \tif (type_ != Type::Value)\n> > \t\treturn std::nullopt;\n> >\n> > -\tif (value_ == \"\")\n> > -\t\treturn std::nullopt;\n> > +\tuint32_t value;\n> >\n> > -\t/*\n> > -\t * libyaml parses all scalar values as strings. When a string has\n> > -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> > -\t * skips leading spaces, accepts the leading minus sign, and the\n> > -\t * calculated digits are negated as if by unary minus. Rule it out in\n> > -\t * case the user gets a large number when the value is negative.\n> > -\t */\n> > -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> > -\tif (found != std::string::npos && value_[found] == '-')\n> > -\t\treturn std::nullopt;\n> > -\n> > -\tchar *end;\n> > -\n> > -\terrno = 0;\n> > -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> > -\n> > -\tif ('\\0' != *end || errno == ERANGE ||\n> > -\t value < std::numeric_limits::min() ||\n> > -\t value > std::numeric_limits::max())\n> > +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> > +\t\t\t\t &value))\n> > \t\treturn std::nullopt;\n> >\n> > \treturn value;\n> > @@ -247,17 +246,10 @@ std::optional YamlObject::get() const\n> > \tif (type_ != Type::Value)\n> > \t\treturn std::nullopt;\n> >\n> > -\tif (value_ == \"\")\n> > -\t\treturn std::nullopt;\n> > +\tint32_t value;\n> >\n> > -\tchar *end;\n> > -\n> > -\terrno = 0;\n> > -\tlong value = std::strtol(value_.c_str(), &end, 10);\n> > -\n> > -\tif ('\\0' != *end || errno == ERANGE ||\n> > -\t value < std::numeric_limits::min() ||\n> > -\t value > std::numeric_limits::max())\n> > +\tif (!parseSignedInteger(value_, std::numeric_limits::min(),\n> > +\t\t\t\tstd::numeric_limits::max(), &value))\n> > \t\treturn std::nullopt;\n> >\n> > \treturn value;\n> > @@ -269,28 +261,10 @@ std::optional YamlObject::get() const\n> > \tif (type_ != Type::Value)\n> > \t\treturn std::nullopt;\n> >\n> > -\tif (value_ == \"\")\n> > -\t\treturn std::nullopt;\n> > +\tuint32_t value;\n> >\n> > -\t/*\n> > -\t * libyaml parses all scalar values as strings. When a string has\n> > -\t * leading spaces before a minus sign, for example \" -10\", strtoul\n> > -\t * skips leading spaces, accepts the leading minus sign, and the\n> > -\t * calculated digits are negated as if by unary minus. Rule it out in\n> > -\t * case the user gets a large number when the value is negative.\n> > -\t */\n> > -\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n> > -\tif (found != std::string::npos && value_[found] == '-')\n> > -\t\treturn std::nullopt;\n> > -\n> > -\tchar *end;\n> > -\n> > -\terrno = 0;\n> > -\tunsigned long value = std::strtoul(value_.c_str(), &end, 10);\n> > -\n> > -\tif ('\\0' != *end || errno == ERANGE ||\n> > -\t value < std::numeric_limits::min() ||\n> > -\t value > std::numeric_limits::max())\n> > +\tif (!parseUnsignedInteger(value_, std::numeric_limits::max(),\n> > +\t\t\t\t &value))\n> > \t\treturn std::nullopt;\n> >\n> > \treturn value;","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 65562BE173\n\tfor ;\n\tThu, 18 Aug 2022 17:41:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A067A61FC0;\n\tThu, 18 Aug 2022 19:41:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 115DC61FA7\n\tfor ;\n\tThu, 18 Aug 2022 19:41:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 76B238B;\n\tThu, 18 Aug 2022 19:41:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660844490;\n\tbh=aXxB5emkhxOubvjdknlauNT46r78ujiOM09s3Ro5b9A=;\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=wVL3u7ZHgJwCR7Y8lgE5xhMSikCApqeG6i/K255I0oRuh00oWoboUjKHh2+ngzotJ\n\tBMa3nXCJinYAigKPZN/KYfbpIXVKYjAhMx++8IPIcRzWs97Krz8sQH7NMHKsYJfqhM\n\teWcH2t/OTTu4Z/czLKY6EajQ+P35O5s71JzfiY2vsTQITGI6SXuOG+IEP7x9egDL9b\n\tqLLcsS8hI1zb13tx1dZ248LjBmDzIArfVo84kXa8wG3EdWp2R41mlyyXIQtNz4nyFZ\n\tBLKgCspbGjX9+DEwNk2qtN6GLB41MjUOhu0XCLK6Zy/ifTCiZz/P50NhNpXKESL+eE\n\tbbN+fk5v7g/Vw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660844488;\n\tbh=aXxB5emkhxOubvjdknlauNT46r78ujiOM09s3Ro5b9A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YPTeD0u40HSbsyI8IwPYUAJZIOqXEfUt+CgKwYMiCf1wechQ+aTTY8i7n93QAkGHZ\n\tK1Yp01Hc7bgRovdm5KiZffj2PNmDXtx6sxwyZu9+7tc1lyXMZ13WI/uFujLwAwd3si\n\t2EqltGJaLtyzdEAdAt65c+ZgCQxmiG+DZlTEen4E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YPTeD0u4\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 20:41:25 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220816015414.7462-1-laurent.pinchart@ideasonboard.com>\n\t<20220816015414.7462-7-laurent.pinchart@ideasonboard.com>\n\t<20220818134434.wg7bq7o5k7gg3dt6@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220818134434.wg7bq7o5k7gg3dt6@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser:\n\tDe-duplicate common code in YamlObject::get()","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]