[libcamera-devel] [PATCH v4 6/9] libcamera: yaml_parser: De-duplicate common code in YamlObject::get()

[Laurent Pinchart]

Hi Jacopo,

On Thu, Aug 18, 2022 at 03:44:34PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 16, 2022 at 04:54:11AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The specializations of the YamlObject::get() function template for
> > integer types duplicate code that doesn't directly depend on the
> > template type argument. Move it to separate helper functions to reduce
> > the object size.
> >
> > While at it, rephrase the comment about unsigned integer parsing.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/yaml_parser.cpp | 160 ++++++++++++++--------------------
> >  1 file changed, 67 insertions(+), 93 deletions(-)
> >
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 85a52c05e682..3d5efdc4419b 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -131,23 +131,65 @@ std::optional<bool> YamlObject::get() const
> >  	return std::nullopt;
> >  }
> >
> > +namespace {
> > +
> > +bool parseSignedInteger(const std::string &str, int32_t min, int32_t max,
> > +			int32_t *result)
> 
> The usage of int32_t types might not scale if we want to add support
> for parsing 64-bit values.

I thought about it and then decided to leave it for later, but I think I
can already change this to use a long (and an unsigned long below) as
those are the return types of strtol and strtoul. It won't give us
64-bit support on 32-bit platforms, that will need to be handled when
adding 64-bit integer support in any case.

> For now I think it's fine
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +{
> > +	if (str == "")
> > +		return false;
> > +
> > +	char *end;
> > +
> > +	errno = 0;
> > +	long value = std::strtol(str.c_str(), &end, 10);
> > +
> > +	if ('\0' != *end || errno == ERANGE || value < min || value > max)
> > +		return false;
> > +
> > +	*result = value;
> > +	return true;
> > +}
> > +
> > +bool parseUnsignedInteger(const std::string &str, uint32_t max, uint32_t *result)
> > +{
> > +	if (str == "")
> > +		return false;
> > +
> > +	/*
> > +	 * strtoul() accepts strings representing a negative number, in which
> > +	 * case it negates the converted value. We don't want to silently accept
> > +	 * negative values and return a large positive number, so check for a
> > +	 * minus sign (after optional whitespace) and return an error.
> > +	 */
> > +	std::size_t found = str.find_first_not_of(" \t");
> > +	if (found != std::string::npos && str[found] == '-')
> > +		return false;
> > +
> > +	char *end;
> > +
> > +	errno = 0;
> > +	unsigned long value = std::strtoul(str.c_str(), &end, 10);
> > +
> > +	if ('\0' != *end || errno == ERANGE || value > max)
> > +		return false;
> > +
> > +	*result = value;
> > +	return true;
> > +}
> > +
> > +} /* namespace */
> > +
> >  template<>
> >  std::optional<int8_t> YamlObject::get() const
> >  {
> >  	if (type_ != Type::Value)
> >  		return std::nullopt;
> >
> > -	if (value_ == "")
> > -		return std::nullopt;
> > +	int32_t value;
> >
> > -	char *end;
> > -
> > -	errno = 0;
> > -	long value = std::strtol(value_.c_str(), &end, 10);
> > -
> > -	if ('\0' != *end || errno == ERANGE ||
> > -	    value < std::numeric_limits<int8_t>::min() ||
> > -	    value > std::numeric_limits<int8_t>::max())
> > +	if (!parseSignedInteger(value_, std::numeric_limits<int8_t>::min(),
> > +				std::numeric_limits<int8_t>::max(), &value))
> >  		return std::nullopt;
> >
> >  	return value;
> > @@ -159,28 +201,10 @@ std::optional<uint8_t> YamlObject::get() const
> >  	if (type_ != Type::Value)
> >  		return std::nullopt;
> >
> > -	if (value_ == "")
> > -		return std::nullopt;
> > +	uint32_t value;
> >
> > -	/*
> > -	 * libyaml parses all scalar values as strings. When a string has
> > -	 * leading spaces before a minus sign, for example "  -10", strtoul
> > -	 * skips leading spaces, accepts the leading minus sign, and the
> > -	 * calculated digits are negated as if by unary minus. Rule it out in
> > -	 * case the user gets a large number when the value is negative.
> > -	 */
> > -	std::size_t found = value_.find_first_not_of(" \t");
> > -	if (found != std::string::npos && value_[found] == '-')
> > -		return std::nullopt;
> > -
> > -	char *end;
> > -
> > -	errno = 0;
> > -	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> > -
> > -	if ('\0' != *end || errno == ERANGE ||
> > -	    value < std::numeric_limits<uint8_t>::min() ||
> > -	    value > std::numeric_limits<uint8_t>::max())
> > +	if (!parseUnsignedInteger(value_, std::numeric_limits<uint8_t>::max(),
> > +				  &value))
> >  		return std::nullopt;
> >
> >  	return value;
> > @@ -192,17 +216,10 @@ std::optional<int16_t> YamlObject::get() const
> >  	if (type_ != Type::Value)
> >  		return std::nullopt;
> >
> > -	if (value_ == "")
> > -		return std::nullopt;
> > +	int32_t value;
> >
> > -	char *end;
> > -
> > -	errno = 0;
> > -	long value = std::strtol(value_.c_str(), &end, 10);
> > -
> > -	if ('\0' != *end || errno == ERANGE ||
> > -	    value < std::numeric_limits<int16_t>::min() ||
> > -	    value > std::numeric_limits<int16_t>::max())
> > +	if (!parseSignedInteger(value_, std::numeric_limits<int16_t>::min(),
> > +				std::numeric_limits<int16_t>::max(), &value))
> >  		return std::nullopt;
> >
> >  	return value;
> > @@ -214,28 +231,10 @@ std::optional<uint16_t> YamlObject::get() const
> >  	if (type_ != Type::Value)
> >  		return std::nullopt;
> >
> > -	if (value_ == "")
> > -		return std::nullopt;
> > +	uint32_t value;
> >
> > -	/*
> > -	 * libyaml parses all scalar values as strings. When a string has
> > -	 * leading spaces before a minus sign, for example "  -10", strtoul
> > -	 * skips leading spaces, accepts the leading minus sign, and the
> > -	 * calculated digits are negated as if by unary minus. Rule it out in
> > -	 * case the user gets a large number when the value is negative.
> > -	 */
> > -	std::size_t found = value_.find_first_not_of(" \t");
> > -	if (found != std::string::npos && value_[found] == '-')
> > -		return std::nullopt;
> > -
> > -	char *end;
> > -
> > -	errno = 0;
> > -	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> > -
> > -	if ('\0' != *end || errno == ERANGE ||
> > -	    value < std::numeric_limits<uint16_t>::min() ||
> > -	    value > std::numeric_limits<uint16_t>::max())
> > +	if (!parseUnsignedInteger(value_, std::numeric_limits<uint16_t>::max(),
> > +				  &value))
> >  		return std::nullopt;
> >
> >  	return value;
> > @@ -247,17 +246,10 @@ std::optional<int32_t> YamlObject::get() const
> >  	if (type_ != Type::Value)
> >  		return std::nullopt;
> >
> > -	if (value_ == "")
> > -		return std::nullopt;
> > +	int32_t value;
> >
> > -	char *end;
> > -
> > -	errno = 0;
> > -	long value = std::strtol(value_.c_str(), &end, 10);
> > -
> > -	if ('\0' != *end || errno == ERANGE ||
> > -	    value < std::numeric_limits<int32_t>::min() ||
> > -	    value > std::numeric_limits<int32_t>::max())
> > +	if (!parseSignedInteger(value_, std::numeric_limits<int32_t>::min(),
> > +				std::numeric_limits<int32_t>::max(), &value))
> >  		return std::nullopt;
> >
> >  	return value;
> > @@ -269,28 +261,10 @@ std::optional<uint32_t> YamlObject::get() const
> >  	if (type_ != Type::Value)
> >  		return std::nullopt;
> >
> > -	if (value_ == "")
> > -		return std::nullopt;
> > +	uint32_t value;
> >
> > -	/*
> > -	 * libyaml parses all scalar values as strings. When a string has
> > -	 * leading spaces before a minus sign, for example "  -10", strtoul
> > -	 * skips leading spaces, accepts the leading minus sign, and the
> > -	 * calculated digits are negated as if by unary minus. Rule it out in
> > -	 * case the user gets a large number when the value is negative.
> > -	 */
> > -	std::size_t found = value_.find_first_not_of(" \t");
> > -	if (found != std::string::npos && value_[found] == '-')
> > -		return std::nullopt;
> > -
> > -	char *end;
> > -
> > -	errno = 0;
> > -	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> > -
> > -	if ('\0' != *end || errno == ERANGE ||
> > -	    value < std::numeric_limits<uint32_t>::min() ||
> > -	    value > std::numeric_limits<uint32_t>::max())
> > +	if (!parseUnsignedInteger(value_, std::numeric_limits<uint32_t>::max(),
> > +				  &value))
> >  		return std::nullopt;
> >
> >  	return value;

-- 
Regards,

Laurent Pinchart