[libcamera-devel,v5,1/3] libcamera: Introduce YamlParser as a helper to parse yaml files

15705 diff mbox series

Introduce YamlParser as a helper to convert contents of a yaml file to
a tree based structure for easier reading, and to avoid writing parser
with raw yaml tokens. The class is based on libyaml, and only support
reading but writing a yaml file.

The interface is inspired by Json::Value class from jsoncpp:
http://jsoncpp.sourceforge.net/class_json_1_1_value.html

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 README.rst                               |   4 +-
 include/libcamera/internal/meson.build   |   1 +
 include/libcamera/internal/yaml_parser.h |  87 +++
 src/libcamera/meson.build                |   3 +
 src/libcamera/yaml_parser.cpp            | 679 +++++++++++++++++++++++
 5 files changed, 772 insertions(+), 2 deletions(-)
 create mode 100644 include/libcamera/internal/yaml_parser.h
 create mode 100644 src/libcamera/yaml_parser.cpp

Hi Han-Lin,

Thank you for the patch.

On Mon, Apr 25, 2022 at 10:46:15PM +0800, Han-Lin Chen via libcamera-devel wrote:
> Introduce YamlParser as a helper to convert contents of a yaml file to
> a tree based structure for easier reading, and to avoid writing parser
> with raw yaml tokens. The class is based on libyaml, and only support
> reading but writing a yaml file.

s/but/but not/ ?

> The interface is inspired by Json::Value class from jsoncpp:
> http://jsoncpp.sourceforge.net/class_json_1_1_value.html
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  README.rst                               |   4 +-
>  include/libcamera/internal/meson.build   |   1 +
>  include/libcamera/internal/yaml_parser.h |  87 +++
>  src/libcamera/meson.build                |   3 +
>  src/libcamera/yaml_parser.cpp            | 679 +++++++++++++++++++++++
>  5 files changed, 772 insertions(+), 2 deletions(-)
>  create mode 100644 include/libcamera/internal/yaml_parser.h
>  create mode 100644 src/libcamera/yaml_parser.cpp
> 
> diff --git a/README.rst b/README.rst
> index aae6b79f..b5a2d448 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -60,7 +60,7 @@ Meson Build system: [required]
>              pip3 install --user --upgrade meson
>  
>  for the libcamera core: [required]
> -        python3-yaml python3-ply python3-jinja2
> +        libyaml-dev python3-yaml python3-ply python3-jinja2
>  
>  for IPA module signing: [required]
>          libgnutls28-dev openssl
> @@ -98,7 +98,7 @@ for tracing with lttng: [optional]
>          liblttng-ust-dev python3-jinja2 lttng-tools
>  
>  for android: [optional]
> -        libexif-dev libjpeg-dev libyaml-dev
> +        libexif-dev libjpeg-dev
>  
>  for lc-compliance: [optional]
>          libevent-dev
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index c9e055d4..7a780d48 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -42,4 +42,5 @@ libcamera_internal_headers = files([
>      'v4l2_pixelformat.h',
>      'v4l2_subdevice.h',
>      'v4l2_videodevice.h',
> +    'yaml_parser.h',
>  ])
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> new file mode 100644
> index 00000000..99f9eb17
> --- /dev/null
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * yaml_parser.h - libcamera YAML parsing helper
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <stdio.h>

This should be cstdio as you use std::FILE (in practice stdio.h also
defines std::FILE in libc++ and libstdc++, but that's not guaranteed by
the standard).

> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +class YamlParserContext;
> +
> +class YamlObject
> +{
> +public:
> +	YamlObject();
> +	~YamlObject();
> +
> +	bool isValue() const
> +	{
> +		return type_ == Value;
> +	}
> +	bool isList() const
> +	{
> +		return type_ == List;
> +	}
> +	bool isDictionary() const
> +	{
> +		return type_ == Dictionary;
> +	}
> +
> +#ifndef __DOXYGEN__
> +	template<typename T,
> +		 typename std::enable_if_t<
> +			 std::is_same<bool, T>::value ||
> +			 std::is_same<double, T>::value ||
> +			 std::is_same<int32_t, T>::value ||
> +			 std::is_same<uint32_t, T>::value ||
> +			 std::is_same<std::string, T>::value ||
> +			 std::is_same<Size, T>::value> * = nullptr>
> +#else
> +	template<typename T>
> +#endif
> +	T get(const T &defaultValue, bool *ok = nullptr) const;
> +
> +	std::size_t size() const;
> +	const YamlObject &operator[](std::size_t index) const;
> +
> +	bool contains(const std::string &key) const;
> +	const YamlObject &operator[](const std::string &key) const;
> +	std::vector<std::string> getMemberNames() const;
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(YamlObject)
> +
> +	friend class YamlParserContext;
> +
> +	enum Type {
> +		Dictionary,
> +		List,
> +		Value,
> +	};
> +
> +	Type type_;
> +
> +	std::string value_;
> +	std::vector<std::unique_ptr<YamlObject>> list_;
> +	std::map<const std::string, std::unique_ptr<YamlObject>> dictionary_;
> +};
> +
> +class YamlParser final
> +{
> +public:
> +	static std::unique_ptr<YamlObject> parse(std::FILE *fh);
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 26912ca1..f8e18e03 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -46,6 +46,7 @@ libcamera_sources = files([
>      'v4l2_pixelformat.cpp',
>      'v4l2_subdevice.cpp',
>      'v4l2_videodevice.cpp',
> +    'yaml_parser.cpp',
>  ])
>  
>  libcamera_sources += libcamera_public_headers
> @@ -66,6 +67,7 @@ subdir('proxy')
>  libdl = cc.find_library('dl')
>  libgnutls = cc.find_library('gnutls', required : true)
>  libudev = dependency('libudev', required : false)
> +libyaml = dependency('yaml-0.1', required : true)
>  
>  if libgnutls.found()
>      config_h.set('HAVE_GNUTLS', 1)
> @@ -126,6 +128,7 @@ libcamera_deps = [
>      libgnutls,
>      liblttng,
>      libudev,
> +    libyaml,
>  ]
>  
>  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> new file mode 100644
> index 00000000..43b83b8f
> --- /dev/null
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -0,0 +1,679 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * yaml_parser.cpp - libcamera YAML parsing helper
> + */
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <yaml.h>
> +
> +/**
> + * \file libcamera/internal/yaml_parser.h
> + * \brief A YAML parser helper
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(YamlParser)
> +
> +namespace {
> +
> +/* Empty static YamlObject as a safe result for invalid operations */
> +static const YamlObject empty;
> +
> +void setOk(bool *ok, bool result)
> +{
> +	if (ok)
> +		*ok = result;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \class YamlObject
> + * \brief A class representing the tree structure of the YAML content
> + *
> + * The YamlObject class represents the tree structure of YAML content. A
> + * YamlObject can be a dictionary or list of YamlObjects or a value if a tree
> + * leaf.
> + */

This documents the class, not the constructor, so you can add a blank
line here to make that clearer.

> +YamlObject::YamlObject()
> +	: type_(Value)
> +{
> +}
> +
> +/**
> + * \class YamlObject

This line should be dropped, this comment block is not about the class,
but about the desctructor. You could write

 * \fn +YamlObject::~YamlObject()

but that's implicit, doxygen uses the name of the next function by
default.

If doxygen doesn't produce any warning, I'd actually drop this comment
block, as it doesn't bring much value.

> + * \brief Destructor of YamlObject
> + */
> +YamlObject::~YamlObject() = default;
> +
> +/**
> + * \fn YamlObject::isValue()
> + * \brief Return whether the YamlObject is a value
> + *
> + * \return True if the YamlObject is a value, false otherwise
> + */
> +
> +/**
> + * \fn YamlObject::isList()
> + * \brief Return whether the YamlObject is a list
> + *
> + * \return True if the YamlObject is a list, false otherwise
> + */
> +
> +/**
> + * \fn YamlObject::isDictionary()
> + * \brief Return whether the YamlObject is a dictionary
> + *
> + * \return True if the YamlObject is a dictionary, false otherwise
> + */
> +
> +/**
> + * \fn template<typename T> YamlObject::get<T>(
> + *	const T &defaultValue, bool *ok) const
> + * \brief Parse the YamlObject as a \a T value
> + * \param[in] defaultValue The default value when failing to parse
> + * \param[out] ok The result of whether the parse succeeded
> + *
> + * This function parses the value of the YamlObject as a \a T object, and
> + * returns the value. If parsing fails (usually because the YamlObject doesn't
> + * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> + * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> + *
> + * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> + * need to know if parsing succeeded.
> + *
> + * \return Value as a bool type
> + */
> +
> +#ifndef __DOXYGEN__
> +
> +template<>
> +bool YamlObject::get(const bool &defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != Value)
> +		return defaultValue;
> +
> +	if (value_ == "true") {
> +		setOk(ok, true);
> +		return true;
> +	} else if (value_ == "false") {
> +		setOk(ok, true);
> +		return false;
> +	}
> +
> +	return defaultValue;
> +}
> +
> +template<>
> +int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != Value)
> +		return defaultValue;
> +
> +	if (value_ == "")
> +		return defaultValue;
> +
> +	char *end;
> +
> +	errno = 0;

You need to include errno.h or cerrno.

> +	int32_t value = std::strtol(value_.c_str(), &end, 10);

And cstdlib.

> +
> +	if ('\0' != *end || errno == ERANGE)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value;
> +}
> +
> +template<>
> +uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != Value)
> +		return defaultValue;
> +
> +	if (value_ == "")
> +		return defaultValue;
> +
> +	/*
> +	 * strtoul accept a leading minus sign, and the calculated digits
> +	 * is negated as if by unary minus. Rule out the case.
> +	 */
> +	std::size_t found = value_.find_first_not_of(" \t");

While those characters shouldn't be present in the data, strtoul will
skip any of the std::isspace() values. We can probably ignore that.

> +	if (found != std::string::npos && value_[found] == '-')
> +		return defaultValue;
> +
> +	char *end;
> +
> +	errno = 0;
> +	uint32_t value = std::strtoul(value_.c_str(), &end, 10);
> +
> +	if ('\0' != *end || errno == ERANGE)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value;
> +}
> +
> +template<>
> +double YamlObject::get(const double &defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != Value)
> +		return defaultValue;
> +
> +	if (value_ == "")
> +		return defaultValue;
> +
> +	char *end;
> +
> +	errno = 0;
> +	double value = std::strtod(value_.c_str(), &end);
> +
> +	if ('\0' != *end || errno == ERANGE)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value;
> +}
> +
> +template<>
> +std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != Value)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value_;
> +}
> +
> +template<>
> +Size YamlObject::get(const Size &defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != List)
> +		return defaultValue;
> +
> +	if (list_.size() != 2)
> +		return defaultValue;
> +
> +	/*
> +	 * Add a local variable to validate of each dimension in case

s/of each/each/

> +	 * that ok == nullptr.
> +	 */
> +	bool valid;
> +	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> +	if (!valid)
> +		return defaultValue;
> +
> +	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> +	if (!valid)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return Size(width, height);
> +}
> +
> +#endif /* __DOXYGEN__ */
> +
> +/**
> + * \fn YamlObject::size()
> + * \brief Retrieve the number of elements in a list YamlObject
> + *
> + * This function retrieves the size of the YamlObject, defined as the number of
> + * child elements it contains. Only YamlObject instances of list type have a

s/list/List/

> + * size, calling this function on other types of instances is invalid and
> + * results in undefined behaviour.
> + *
> + * \return The size of the YamlObject
> + */
> +std::size_t YamlObject::size() const
> +{
> +	if (type_ != List)
> +		return 0;
> +
> +	return list_.size();
> +}
> +
> +/**
> + * \fn YamlObject::operator[](std::size_t index) const
> + * \brief Retrieve the element from list YamlObject by index
> + *
> + * This function retrieves an element of the YamlObject. Only YamlObject
> + * instances of list type associate elements with index, calling this function

s/list/List/

> + * on other types of instances is invalid and results in undefined behaviour.
> + *
> + * \return The YamlObject as an element of the list
> + */
> +const YamlObject &YamlObject::operator[](std::size_t index) const
> +{
> +	if (type_ != List || index >= size())
> +		return empty;
> +
> +	return *list_[index];
> +}
> +
> +/**
> + * \fn YamlObject::contains()
> + * \brief Check if an element of a dictionary exists
> + *
> + * This function check if the YamlObject contains an element. Only YamlObject
> + * instances of dictionary type associate elements with names, calling this

s/dictionary/Dictionary/

> + * function on other types of instances is invalid and results in undefined
> + * behaviour.
> + *
> + * \return True if an element exists, false otherwise
> + */
> +bool YamlObject::contains(const std::string &key) const
> +{
> +	if (dictionary_.find(key) == dictionary_.end())
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * \fn YamlObject::getMemberNames()

s/getMemberNames()/memberNames()/ as we don't usually prefix getters
with "get".

> + * \brief Retrieve all member names of the dictionary
> + *
> + * This function retrieve member names of a YamlObject. Only YamlObject
> + * instances of dictionary type associate elements with names, calling this

s/dictionary/Dictionary/

> + * function on other types of instances is invalid and results in undefined
> + * behaviour.
> + *
> + * \todo Replace this function with an iterator-based API
> + *
> + * \return A vector of string as the member names
> + */
> +std::vector<std::string> YamlObject::getMemberNames() const
> +{
> +	std::vector<std::string> memberNames;
> +	for (auto &[key, _] : dictionary_)
> +		memberNames.push_back(key);
> +
> +	return memberNames;
> +}
> +
> +/**
> + * \fn YamlObject::operator[](const std::string &key) const
> + * \brief Retrieve a member by name from the dictionary
> + *
> + * This function retrieve a member of a YamlObject by name. Only YamlObject
> + * instances of dictionary type associate elements with names, calling this

s/dictionary/Dictionary/

> + * function on other types of instances is invalid and results in undefined
> + * behaviour.
> + *
> + * \return The YamlObject corresponding to the \a key member
> + */
> +const YamlObject &YamlObject::operator[](const std::string &key) const
> +{
> +	if (type_ != Dictionary || !contains(key))
> +		return empty;
> +
> +	auto iter = dictionary_.find(key);
> +	return *iter->second;
> +}
> +
> +#ifndef __DOXYGEN__
> +
> +class YamlParserContext
> +{
> +public:
> +	YamlParserContext();
> +	~YamlParserContext();
> +
> +	int init(std::FILE *fh);
> +	int parseContent(YamlObject &yamlObject);
> +
> +private:
> +	struct EventDeleter {
> +		void operator()(yaml_event_t *event) const
> +		{
> +			yaml_event_delete(event);
> +			delete event;
> +		}
> +	};
> +	using EventPtr = std::unique_ptr<yaml_event_t, EventDeleter>;

Nice, I like this :-)

> +
> +	EventPtr getEvent();

Maybe nextEvent() ? Up to you.

> +
> +	void readValue(std::string &value, EventPtr event);
> +	int parseDictionaryOrList(bool isDictionary,
> +				  const std::function<int(EventPtr event)> &parseItem);
> +	int parseNextYamlObject(YamlObject &yamlObject, EventPtr event);
> +
> +	bool parserValid_;
> +	yaml_parser_t parser_;
> +};
> +
> +/**
> + * \class YamlParserContext
> + * \brief Class for YamlParser parsing and context data
> + *
> + * The YamlParserContext class stores the internal yaml_parser_t and provides
> + * helper functions to do event-based parsing for YAML files.
> + */
> +YamlParserContext::YamlParserContext()
> +	: parserValid_(false)
> +{
> +}
> +
> +/**
> + * \class YamlParserContext
> + * \brief Destructor of YamlParserContext
> + */
> +YamlParserContext::~YamlParserContext()
> +{
> +	if (parserValid_) {
> +		yaml_parser_delete(&parser_);
> +		parserValid_ = false;
> +	}
> +}
> +
> +/**
> + * \fn YamlParserContext::init()
> + * \brief Initialize a parser with an opened file for parsing
> + * \param[in] fh The YAML file to parse
> + *
> + * Prior to parsing the YAML content, the YamlParserContext must be initialized
> + * with an opened FILE to create an internal parser. The FILE needs to stay
> + * valid during the process.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser has failed to initialize
> + */
> +int YamlParserContext::init(std::FILE *fh)
> +{
> +	/* yaml_parser_initialize returns 1 when it succeededs */
> +	if (!yaml_parser_initialize(&parser_)) {
> +		LOG(YamlParser, Error) << "Failed to initialize YAML parser";
> +		return -EINVAL;
> +	}
> +	parserValid_ = true;
> +	yaml_parser_set_input_file(&parser_, fh);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParserContext::getEvent()
> + * \brief Get the next event
> + *
> + * Get the next event in the current YAML event stream, and return nullptr when
> + * there is no more event.
> + *
> + * \return EventPtr on success or nullptr otherwise

s/EventPtr/The next event/

> + */
> +YamlParserContext::EventPtr YamlParserContext::getEvent()
> +{
> +	static yaml_event_t event;
> +
> +	/* yaml_parser_parse returns when it succeeds */
> +	if (1 != yaml_parser_parse(&parser_, &event)) {

You could write

	if (!yaml_parser_parse(&parser_, &event))

to match YamlParserContext::init(), unless there is a need to check for
other values than 0 and 1.

> +		return nullptr;
> +	}

No need for curly braces.

> +
> +	return EventPtr(new yaml_event_t(event));

Would the following help avoiding copies ?

	EventPtr event = std::make_unique<yaml_event_t>();

	if (!yaml_parser_parse(&parser_, event.get()))
		return nullptr;

	return event;

> +}
> +
> +/**
> + * \fn YamlParserContext::parseContent()
> + * \brief Parse the content of a YAML document
> + * \param[in] yamlObject The result of YamlObject
> + *
> + * Check YAML start and end events of a YAML document, and parse the root object
> + * of the YAML document into a YamlObject.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to validate end of a YAML file

s/is failed/has failed/

> + */
> +int YamlParserContext::parseContent(YamlObject &yamlObject)
> +{
> +	/* Check start of the YAML file */

s/file/file./

Same below.

> +	EventPtr event = getEvent();
> +	if (!event || event->type != YAML_STREAM_START_EVENT)
> +		return -EINVAL;
> +
> +	event = getEvent();
> +	if (!event || event->type != YAML_DOCUMENT_START_EVENT)
> +		return -EINVAL;
> +
> +	/* Parse the root object */
> +	event = getEvent();
> +	if (parseNextYamlObject(yamlObject, std::move(event)))
> +		return -EINVAL;
> +
> +	/* Check end of the YAML file */
> +	event = getEvent();
> +	if (!event || event->type != YAML_DOCUMENT_END_EVENT)
> +		return -EINVAL;
> +
> +	event = getEvent();
> +	if (!event || event->type != YAML_STREAM_END_EVENT)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParserContext::readValue()
> + * \brief Parse event scalar and fill its content into a string
> + * \param[in] value The string reference to fill value
> + *
> + * A helper function to parse a scalar event as string. The caller need to

s/need/needs/

> + * guarantee the event is of scaler type.
> + */
> +void YamlParserContext::readValue(std::string &value, EventPtr event)
> +{
> +	value.assign(reinterpret_cast<char *>(event->data.scalar.value),
> +		     event->data.scalar.length);
> +}
> +
> +/**
> + * \fn YamlParserContext::parseDictionaryOrList()
> + * \brief A helper function to abstract common part of parsing dictionary or list

s/common part/the common part/

> + *
> + * \param[in] isDictionary True for parsing a dictionary, and false for a list

Could you pass a yaml_event_type_t instead and name the parameter
endEventType ? That would make the callers more readable. Another option
would be to pass a YamlObject::Type.

> + * \param[in] parseItem The callback to handle an item
> + *
> + * A helper function to abstract parsing a item from a dictionary or a list.

s/a item/an item/

> + * The differences of them in a YAML event stream are:
> + *
> + * 1. The start and end event type are different

s/type/types/

> + * 2. There is a leading scalar string as key in the items of a dictionary
> + *
> + * The caller should handle the leading key string in its callback parseItem
> + * when it's a dictionary.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to initialize
> + */
> +int YamlParserContext::parseDictionaryOrList(bool isDictionary,
> +					     const std::function<int(EventPtr event)> &parseItem)
> +{
> +	yaml_event_type_t endEventType = YAML_SEQUENCE_END_EVENT;
> +	if (isDictionary)
> +		endEventType = YAML_MAPPING_END_EVENT;
> +
> +	/*
> +	 * Add a safety counter to make sure we don't loop indefinitely in case
> +	 * the configuration file is malformed.
> +	 */
> +	unsigned int sentinel = 1000;
> +	do {

	for (unsigned int sentinel = 1000; sentinel; --sentinel) {

> +		auto evt = getEvent();
> +		if (!evt)
> +			return -EINVAL;
> +
> +		if (evt->type == endEventType)
> +			return 0;
> +
> +		int ret = parseItem(std::move(evt));
> +		if (ret)
> +			return ret;
> +
> +		--sentinel;

and drop this.

> +	} while (sentinel);
> +
> +	if (!sentinel)
> +		return -EINVAL;

You can return -EINVAL unconditionally.

I wonder if there's a need for more than 1000 items in a dictionary or
list. I could imagine that a list storing a LUT (for instance a lens
shading correction table) could grow big. Maybe that will make parsing
very inefficient though, and we'll need a binary format instead at that
point ? For now I suppose the limit is fine, but please add a LOG(Error)

> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParserContext::parseNextYamlObject()
> + * \brief Parse next YAML event and read it as a YamlObject
> + * \param[in] yamlObject The result of YamlObject
> + * \param[in] event The leading event of the object
> + *
> + * Parse next YAML object separately as a value, list or dictionary.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL Fail to parse the YAML file.
> + */
> +int YamlParserContext::parseNextYamlObject(YamlObject &yamlObject, EventPtr event)
> +{
> +	if (!event)
> +		return -EINVAL;
> +
> +	switch (event->type) {
> +	case YAML_SCALAR_EVENT:
> +		yamlObject.type_ = YamlObject::Value;
> +		readValue(yamlObject.value_, std::move(event));
> +		return 0;
> +
> +	case YAML_SEQUENCE_START_EVENT: {
> +		yamlObject.type_ = YamlObject::List;
> +		auto &list = yamlObject.list_;
> +		auto handler = [this, &list](EventPtr evt) {
> +			list.emplace_back(new YamlObject());
> +			return parseNextYamlObject(*list.back(), std::move(evt));
> +		};
> +		return parseDictionaryOrList(false, handler);
> +	}
> +
> +	case YAML_MAPPING_START_EVENT: {
> +		yamlObject.type_ = YamlObject::Dictionary;
> +		auto &dictionary = yamlObject.dictionary_;
> +		auto handler = [this, &dictionary](EventPtr evtKey) {
> +			/* Parse key */
> +			if (evtKey->type != YAML_SCALAR_EVENT) {
> +				LOG(YamlParser, Error) << "Expect key at line: "
> +						       << evtKey->start_mark.line
> +						       << " column: "
> +						       << evtKey->start_mark.column;
> +				return -EINVAL;
> +			}
> +
> +			std::string key;
> +			readValue(key, std::move(evtKey));
> +
> +			/* Parse value */
> +			EventPtr evtValue = getEvent();
> +			if (!evtValue)
> +				return -EINVAL;
> +
> +			dictionary[key].reset(new YamlObject());
> +			return parseNextYamlObject(*dictionary[key], std::move(evtValue));

Let's avoid the double lookup:

			auto elem = dictionary.emplace(key, std::make_unique<YamlObject>());
			return parseNextYamlObject(elem.first->second.get(), std::move(evtValue));

(untested)

> +		};
> +		return parseDictionaryOrList(true, handler);
> +	}

Missing blank line.

> +	default:
> +		LOG(YamlParser, Error) << "Invalid YAML file";

You can move the return -EINVAL here.

> +	}
> +
> +	return -EINVAL;
> +}
> +
> +#endif /* __DOXYGEN__ */
> +
> +/**
> + * \class YamlParser
> + * \brief A helper class for parsing YAML file

s/YAML file/a YAML file/

> + *
> + * The YamlParser class provides an easy interface to parse the contents of a
> + * YAML file int a tree of YamlObject instances.

s/int a/into a/

> + *
> + * Example usage:
> + *
> + * \code{.unparsed}
> + *
> + * name:
> + * 	"John"
> + * numbers:
> + * 	- 1
> + * 	- 2
> + *
> + * \endcode
> + *
> + * The following code illustrates how to parse the above YAML file:
> + *
> + * \code{.cpp}
> + *
> + * std::unique_ptr<YamlObject> root = YamlParser::parse(fh);
> + * if (!root)
> + *   return;
> + *
> + * if (!root->isDictionary())
> + *   return;
> + *
> + * const YamlObject &name = (*root)["name"];
> + * std::cout << name.get<std::string>("");

 * std::cout << name.get<std::string>("") << std:endl();

> + *
> + * const YamlObject &numbers = (*root)["numbers"];
> + * if (!numbers.isList())
> + *   return;
> + *
> + * for (std::size_t i = 0; i < numbers.size(); i++)
> + *   std::cout << numbers[i].get<int32_t>(0);

Same here.

> + *
> + * \endcode
> + *
> + * The YamlParser::parse() function takes open FILE, parses its contents, and

s/open/an open/

> + * returns a pointer to a YamlObject corresponding to the root node of the YAML
> + * document.
> + */
> +
> +/**
> + * \fn YamlParser::parse()
> + * \brief Parse a YAML file as a YamlObject
> + * \param[in] fh The YAML file to parse
> + *
> + * The YamlParser::parse() function takes open FILE, parses its contents, and

s/open/an open/

> + * returns a pointer to a YamlObject corresponding to the root node of the YAML
> + * document.


And I'd add "The caller is responsible for closing the file.".

> + *
> + * \return Pointer to result YamlObject on success or nullptr otherwise
> + */
> +std::unique_ptr<YamlObject> YamlParser::parse(std::FILE *fh)
> +{
> +	YamlParserContext context;
> +
> +	if (context.init(fh))
> +		return nullptr;
> +
> +	std::unique_ptr<YamlObject> root(new YamlObject());
> +
> +	if (context.parseContent(*root))

Please LOG() an error here.

Overall this looks good, comments are minor. I expect to merge v6. Thank
you for your work !

> +		return nullptr;
> +
> +	return root;
> +}
> +
> +} /* namespace libcamera */