[{"id":22802,"web_url":"https://patchwork.libcamera.org/comment/22802/","msgid":"","date":"2022-04-27T01:12:55","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce\n\tYamlParser as a helper to parse yaml files","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-Lin,\n\nThank you for the patch.\n\nOn Mon, Apr 25, 2022 at 10:46:15PM +0800, Han-Lin Chen via libcamera-devel wrote:\n> Introduce YamlParser as a helper to convert contents of a yaml file to\n> a tree based structure for easier reading, and to avoid writing parser\n> with raw yaml tokens. The class is based on libyaml, and only support\n> reading but writing a yaml file.\n\ns/but/but not/ ?\n\n> The interface is inspired by Json::Value class from jsoncpp:\n> http://jsoncpp.sourceforge.net/class_json_1_1_value.html\n> \n> Signed-off-by: Han-Lin Chen \n> ---\n> README.rst | 4 +-\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/yaml_parser.h | 87 +++\n> src/libcamera/meson.build | 3 +\n> src/libcamera/yaml_parser.cpp | 679 +++++++++++++++++++++++\n> 5 files changed, 772 insertions(+), 2 deletions(-)\n> create mode 100644 include/libcamera/internal/yaml_parser.h\n> create mode 100644 src/libcamera/yaml_parser.cpp\n> \n> diff --git a/README.rst b/README.rst\n> index aae6b79f..b5a2d448 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -60,7 +60,7 @@ Meson Build system: [required]\n> pip3 install --user --upgrade meson\n> \n> for the libcamera core: [required]\n> - python3-yaml python3-ply python3-jinja2\n> + libyaml-dev python3-yaml python3-ply python3-jinja2\n> \n> for IPA module signing: [required]\n> libgnutls28-dev openssl\n> @@ -98,7 +98,7 @@ for tracing with lttng: [optional]\n> liblttng-ust-dev python3-jinja2 lttng-tools\n> \n> for android: [optional]\n> - libexif-dev libjpeg-dev libyaml-dev\n> + libexif-dev libjpeg-dev\n> \n> for lc-compliance: [optional]\n> libevent-dev\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index c9e055d4..7a780d48 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -42,4 +42,5 @@ libcamera_internal_headers = files([\n> 'v4l2_pixelformat.h',\n> 'v4l2_subdevice.h',\n> 'v4l2_videodevice.h',\n> + 'yaml_parser.h',\n> ])\n> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h\n> new file mode 100644\n> index 00000000..99f9eb17\n> --- /dev/null\n> +++ b/include/libcamera/internal/yaml_parser.h\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * yaml_parser.h - libcamera YAML parsing helper\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n\nThis should be cstdio as you use std::FILE (in practice stdio.h also\ndefines std::FILE in libc++ and libstdc++, but that's not guaranteed by\nthe standard).\n\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class YamlParserContext;\n> +\n> +class YamlObject\n> +{\n> +public:\n> +\tYamlObject();\n> +\t~YamlObject();\n> +\n> +\tbool isValue() const\n> +\t{\n> +\t\treturn type_ == Value;\n> +\t}\n> +\tbool isList() const\n> +\t{\n> +\t\treturn type_ == List;\n> +\t}\n> +\tbool isDictionary() const\n> +\t{\n> +\t\treturn type_ == Dictionary;\n> +\t}\n> +\n> +#ifndef __DOXYGEN__\n> +\ttemplate +\t\t typename std::enable_if_t<\n> +\t\t\t std::is_same::value ||\n> +\t\t\t std::is_same::value ||\n> +\t\t\t std::is_same::value ||\n> +\t\t\t std::is_same::value ||\n> +\t\t\t std::is_same::value ||\n> +\t\t\t std::is_same::value> * = nullptr>\n> +#else\n> +\ttemplate\n> +#endif\n> +\tT get(const T &defaultValue, bool *ok = nullptr) const;\n> +\n> +\tstd::size_t size() const;\n> +\tconst YamlObject &operator[](std::size_t index) const;\n> +\n> +\tbool contains(const std::string &key) const;\n> +\tconst YamlObject &operator[](const std::string &key) const;\n> +\tstd::vector getMemberNames() const;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(YamlObject)\n> +\n> +\tfriend class YamlParserContext;\n> +\n> +\tenum Type {\n> +\t\tDictionary,\n> +\t\tList,\n> +\t\tValue,\n> +\t};\n> +\n> +\tType type_;\n> +\n> +\tstd::string value_;\n> +\tstd::vector> list_;\n> +\tstd::map> dictionary_;\n> +};\n> +\n> +class YamlParser final\n> +{\n> +public:\n> +\tstatic std::unique_ptr parse(std::FILE *fh);\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 26912ca1..f8e18e03 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -46,6 +46,7 @@ libcamera_sources = files([\n> 'v4l2_pixelformat.cpp',\n> 'v4l2_subdevice.cpp',\n> 'v4l2_videodevice.cpp',\n> + 'yaml_parser.cpp',\n> ])\n> \n> libcamera_sources += libcamera_public_headers\n> @@ -66,6 +67,7 @@ subdir('proxy')\n> libdl = cc.find_library('dl')\n> libgnutls = cc.find_library('gnutls', required : true)\n> libudev = dependency('libudev', required : false)\n> +libyaml = dependency('yaml-0.1', required : true)\n> \n> if libgnutls.found()\n> config_h.set('HAVE_GNUTLS', 1)\n> @@ -126,6 +128,7 @@ libcamera_deps = [\n> libgnutls,\n> liblttng,\n> libudev,\n> + libyaml,\n> ]\n> \n> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.\n> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> new file mode 100644\n> index 00000000..43b83b8f\n> --- /dev/null\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -0,0 +1,679 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * yaml_parser.cpp - libcamera YAML parsing helper\n> + */\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file libcamera/internal/yaml_parser.h\n> + * \\brief A YAML parser helper\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(YamlParser)\n> +\n> +namespace {\n> +\n> +/* Empty static YamlObject as a safe result for invalid operations */\n> +static const YamlObject empty;\n> +\n> +void setOk(bool *ok, bool result)\n> +{\n> +\tif (ok)\n> +\t\t*ok = result;\n> +}\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\class YamlObject\n> + * \\brief A class representing the tree structure of the YAML content\n> + *\n> + * The YamlObject class represents the tree structure of YAML content. A\n> + * YamlObject can be a dictionary or list of YamlObjects or a value if a tree\n> + * leaf.\n> + */\n\nThis documents the class, not the constructor, so you can add a blank\nline here to make that clearer.\n\n> +YamlObject::YamlObject()\n> +\t: type_(Value)\n> +{\n> +}\n> +\n> +/**\n> + * \\class YamlObject\n\nThis line should be dropped, this comment block is not about the class,\nbut about the desctructor. You could write\n\n * \\fn +YamlObject::~YamlObject()\n\nbut that's implicit, doxygen uses the name of the next function by\ndefault.\n\nIf doxygen doesn't produce any warning, I'd actually drop this comment\nblock, as it doesn't bring much value.\n\n> + * \\brief Destructor of YamlObject\n> + */\n> +YamlObject::~YamlObject() = default;\n> +\n> +/**\n> + * \\fn YamlObject::isValue()\n> + * \\brief Return whether the YamlObject is a value\n> + *\n> + * \\return True if the YamlObject is a value, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn YamlObject::isList()\n> + * \\brief Return whether the YamlObject is a list\n> + *\n> + * \\return True if the YamlObject is a list, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn YamlObject::isDictionary()\n> + * \\brief Return whether the YamlObject is a dictionary\n> + *\n> + * \\return True if the YamlObject is a dictionary, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn template YamlObject::get(\n> + *\tconst T &defaultValue, bool *ok) const\n> + * \\brief Parse the YamlObject as a \\a T value\n> + * \\param[in] defaultValue The default value when failing to parse\n> + * \\param[out] ok The result of whether the parse succeeded\n> + *\n> + * This function parses the value of the YamlObject as a \\a T object, and\n> + * returns the value. If parsing fails (usually because the YamlObject doesn't\n> + * store a \\a T value), the \\a defaultValue is returned, and \\a ok is set to\n> + * false. Otherwise, the YamlObject value is returned, and \\a ok is set to true.\n> + *\n> + * The \\a ok pointer is optional and can be a nullptr if the caller doesn't\n> + * need to know if parsing succeeded.\n> + *\n> + * \\return Value as a bool type\n> + */\n> +\n> +#ifndef __DOXYGEN__\n> +\n> +template<>\n> +bool YamlObject::get(const bool &defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != Value)\n> +\t\treturn defaultValue;\n> +\n> +\tif (value_ == \"true\") {\n> +\t\tsetOk(ok, true);\n> +\t\treturn true;\n> +\t} else if (value_ == \"false\") {\n> +\t\tsetOk(ok, true);\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn defaultValue;\n> +}\n> +\n> +template<>\n> +int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != Value)\n> +\t\treturn defaultValue;\n> +\n> +\tif (value_ == \"\")\n> +\t\treturn defaultValue;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n\nYou need to include errno.h or cerrno.\n\n> +\tint32_t value = std::strtol(value_.c_str(), &end, 10);\n\nAnd cstdlib.\n\n> +\n> +\tif ('\\0' != *end || errno == ERANGE)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value;\n> +}\n> +\n> +template<>\n> +uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != Value)\n> +\t\treturn defaultValue;\n> +\n> +\tif (value_ == \"\")\n> +\t\treturn defaultValue;\n> +\n> +\t/*\n> +\t * strtoul accept a leading minus sign, and the calculated digits\n> +\t * is negated as if by unary minus. Rule out the case.\n> +\t */\n> +\tstd::size_t found = value_.find_first_not_of(\" \\t\");\n\nWhile those characters shouldn't be present in the data, strtoul will\nskip any of the std::isspace() values. We can probably ignore that.\n\n> +\tif (found != std::string::npos && value_[found] == '-')\n> +\t\treturn defaultValue;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n> +\tuint32_t value = std::strtoul(value_.c_str(), &end, 10);\n> +\n> +\tif ('\\0' != *end || errno == ERANGE)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value;\n> +}\n> +\n> +template<>\n> +double YamlObject::get(const double &defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != Value)\n> +\t\treturn defaultValue;\n> +\n> +\tif (value_ == \"\")\n> +\t\treturn defaultValue;\n> +\n> +\tchar *end;\n> +\n> +\terrno = 0;\n> +\tdouble value = std::strtod(value_.c_str(), &end);\n> +\n> +\tif ('\\0' != *end || errno == ERANGE)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value;\n> +}\n> +\n> +template<>\n> +std::string YamlObject::get(const std::string &defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != Value)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value_;\n> +}\n> +\n> +template<>\n> +Size YamlObject::get(const Size &defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != List)\n> +\t\treturn defaultValue;\n> +\n> +\tif (list_.size() != 2)\n> +\t\treturn defaultValue;\n> +\n> +\t/*\n> +\t * Add a local variable to validate of each dimension in case\n\ns/of each/each/\n\n> +\t * that ok == nullptr.\n> +\t */\n> +\tbool valid;\n> +\tuint32_t width = list_[0]->get(0, &valid);\n> +\tif (!valid)\n> +\t\treturn defaultValue;\n> +\n> +\tuint32_t height = list_[1]->get(0, &valid);\n> +\tif (!valid)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn Size(width, height);\n> +}\n> +\n> +#endif /* __DOXYGEN__ */\n> +\n> +/**\n> + * \\fn YamlObject::size()\n> + * \\brief Retrieve the number of elements in a list YamlObject\n> + *\n> + * This function retrieves the size of the YamlObject, defined as the number of\n> + * child elements it contains. Only YamlObject instances of list type have a\n\ns/list/List/\n\n> + * size, calling this function on other types of instances is invalid and\n> + * results in undefined behaviour.\n> + *\n> + * \\return The size of the YamlObject\n> + */\n> +std::size_t YamlObject::size() const\n> +{\n> +\tif (type_ != List)\n> +\t\treturn 0;\n> +\n> +\treturn list_.size();\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::operator[](std::size_t index) const\n> + * \\brief Retrieve the element from list YamlObject by index\n> + *\n> + * This function retrieves an element of the YamlObject. Only YamlObject\n> + * instances of list type associate elements with index, calling this function\n\ns/list/List/\n\n> + * on other types of instances is invalid and results in undefined behaviour.\n> + *\n> + * \\return The YamlObject as an element of the list\n> + */\n> +const YamlObject &YamlObject::operator[](std::size_t index) const\n> +{\n> +\tif (type_ != List || index >= size())\n> +\t\treturn empty;\n> +\n> +\treturn *list_[index];\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::contains()\n> + * \\brief Check if an element of a dictionary exists\n> + *\n> + * This function check if the YamlObject contains an element. Only YamlObject\n> + * instances of dictionary type associate elements with names, calling this\n\ns/dictionary/Dictionary/\n\n> + * function on other types of instances is invalid and results in undefined\n> + * behaviour.\n> + *\n> + * \\return True if an element exists, false otherwise\n> + */\n> +bool YamlObject::contains(const std::string &key) const\n> +{\n> +\tif (dictionary_.find(key) == dictionary_.end())\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::getMemberNames()\n\ns/getMemberNames()/memberNames()/ as we don't usually prefix getters\nwith \"get\".\n\n> + * \\brief Retrieve all member names of the dictionary\n> + *\n> + * This function retrieve member names of a YamlObject. Only YamlObject\n> + * instances of dictionary type associate elements with names, calling this\n\ns/dictionary/Dictionary/\n\n> + * function on other types of instances is invalid and results in undefined\n> + * behaviour.\n> + *\n> + * \\todo Replace this function with an iterator-based API\n> + *\n> + * \\return A vector of string as the member names\n> + */\n> +std::vector YamlObject::getMemberNames() const\n> +{\n> +\tstd::vector memberNames;\n> +\tfor (auto &[key, _] : dictionary_)\n> +\t\tmemberNames.push_back(key);\n> +\n> +\treturn memberNames;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::operator[](const std::string &key) const\n> + * \\brief Retrieve a member by name from the dictionary\n> + *\n> + * This function retrieve a member of a YamlObject by name. Only YamlObject\n> + * instances of dictionary type associate elements with names, calling this\n\ns/dictionary/Dictionary/\n\n> + * function on other types of instances is invalid and results in undefined\n> + * behaviour.\n> + *\n> + * \\return The YamlObject corresponding to the \\a key member\n> + */\n> +const YamlObject &YamlObject::operator[](const std::string &key) const\n> +{\n> +\tif (type_ != Dictionary || !contains(key))\n> +\t\treturn empty;\n> +\n> +\tauto iter = dictionary_.find(key);\n> +\treturn *iter->second;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +\n> +class YamlParserContext\n> +{\n> +public:\n> +\tYamlParserContext();\n> +\t~YamlParserContext();\n> +\n> +\tint init(std::FILE *fh);\n> +\tint parseContent(YamlObject &yamlObject);\n> +\n> +private:\n> +\tstruct EventDeleter {\n> +\t\tvoid operator()(yaml_event_t *event) const\n> +\t\t{\n> +\t\t\tyaml_event_delete(event);\n> +\t\t\tdelete event;\n> +\t\t}\n> +\t};\n> +\tusing EventPtr = std::unique_ptr;\n\nNice, I like this :-)\n\n> +\n> +\tEventPtr getEvent();\n\nMaybe nextEvent() ? Up to you.\n\n> +\n> +\tvoid readValue(std::string &value, EventPtr event);\n> +\tint parseDictionaryOrList(bool isDictionary,\n> +\t\t\t\t const std::function &parseItem);\n> +\tint parseNextYamlObject(YamlObject &yamlObject, EventPtr event);\n> +\n> +\tbool parserValid_;\n> +\tyaml_parser_t parser_;\n> +};\n> +\n> +/**\n> + * \\class YamlParserContext\n> + * \\brief Class for YamlParser parsing and context data\n> + *\n> + * The YamlParserContext class stores the internal yaml_parser_t and provides\n> + * helper functions to do event-based parsing for YAML files.\n> + */\n> +YamlParserContext::YamlParserContext()\n> +\t: parserValid_(false)\n> +{\n> +}\n> +\n> +/**\n> + * \\class YamlParserContext\n> + * \\brief Destructor of YamlParserContext\n> + */\n> +YamlParserContext::~YamlParserContext()\n> +{\n> +\tif (parserValid_) {\n> +\t\tyaml_parser_delete(&parser_);\n> +\t\tparserValid_ = false;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\fn YamlParserContext::init()\n> + * \\brief Initialize a parser with an opened file for parsing\n> + * \\param[in] fh The YAML file to parse\n> + *\n> + * Prior to parsing the YAML content, the YamlParserContext must be initialized\n> + * with an opened FILE to create an internal parser. The FILE needs to stay\n> + * valid during the process.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser has failed to initialize\n> + */\n> +int YamlParserContext::init(std::FILE *fh)\n> +{\n> +\t/* yaml_parser_initialize returns 1 when it succeededs */\n> +\tif (!yaml_parser_initialize(&parser_)) {\n> +\t\tLOG(YamlParser, Error) << \"Failed to initialize YAML parser\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tparserValid_ = true;\n> +\tyaml_parser_set_input_file(&parser_, fh);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParserContext::getEvent()\n> + * \\brief Get the next event\n> + *\n> + * Get the next event in the current YAML event stream, and return nullptr when\n> + * there is no more event.\n> + *\n> + * \\return EventPtr on success or nullptr otherwise\n\ns/EventPtr/The next event/\n\n> + */\n> +YamlParserContext::EventPtr YamlParserContext::getEvent()\n> +{\n> +\tstatic yaml_event_t event;\n> +\n> +\t/* yaml_parser_parse returns when it succeeds */\n> +\tif (1 != yaml_parser_parse(&parser_, &event)) {\n\nYou could write\n\n\tif (!yaml_parser_parse(&parser_, &event))\n\nto match YamlParserContext::init(), unless there is a need to check for\nother values than 0 and 1.\n\n> +\t\treturn nullptr;\n> +\t}\n\nNo need for curly braces.\n\n> +\n> +\treturn EventPtr(new yaml_event_t(event));\n\nWould the following help avoiding copies ?\n\n\tEventPtr event = std::make_unique();\n\n\tif (!yaml_parser_parse(&parser_, event.get()))\n\t\treturn nullptr;\n\n\treturn event;\n\n> +}\n> +\n> +/**\n> + * \\fn YamlParserContext::parseContent()\n> + * \\brief Parse the content of a YAML document\n> + * \\param[in] yamlObject The result of YamlObject\n> + *\n> + * Check YAML start and end events of a YAML document, and parse the root object\n> + * of the YAML document into a YamlObject.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to validate end of a YAML file\n\ns/is failed/has failed/\n\n> + */\n> +int YamlParserContext::parseContent(YamlObject &yamlObject)\n> +{\n> +\t/* Check start of the YAML file */\n\ns/file/file./\n\nSame below.\n\n> +\tEventPtr event = getEvent();\n> +\tif (!event || event->type != YAML_STREAM_START_EVENT)\n> +\t\treturn -EINVAL;\n> +\n> +\tevent = getEvent();\n> +\tif (!event || event->type != YAML_DOCUMENT_START_EVENT)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Parse the root object */\n> +\tevent = getEvent();\n> +\tif (parseNextYamlObject(yamlObject, std::move(event)))\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Check end of the YAML file */\n> +\tevent = getEvent();\n> +\tif (!event || event->type != YAML_DOCUMENT_END_EVENT)\n> +\t\treturn -EINVAL;\n> +\n> +\tevent = getEvent();\n> +\tif (!event || event->type != YAML_STREAM_END_EVENT)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParserContext::readValue()\n> + * \\brief Parse event scalar and fill its content into a string\n> + * \\param[in] value The string reference to fill value\n> + *\n> + * A helper function to parse a scalar event as string. The caller need to\n\ns/need/needs/\n\n> + * guarantee the event is of scaler type.\n> + */\n> +void YamlParserContext::readValue(std::string &value, EventPtr event)\n> +{\n> +\tvalue.assign(reinterpret_cast(event->data.scalar.value),\n> +\t\t event->data.scalar.length);\n> +}\n> +\n> +/**\n> + * \\fn YamlParserContext::parseDictionaryOrList()\n> + * \\brief A helper function to abstract common part of parsing dictionary or list\n\ns/common part/the common part/\n\n> + *\n> + * \\param[in] isDictionary True for parsing a dictionary, and false for a list\n\nCould you pass a yaml_event_type_t instead and name the parameter\nendEventType ? That would make the callers more readable. Another option\nwould be to pass a YamlObject::Type.\n\n> + * \\param[in] parseItem The callback to handle an item\n> + *\n> + * A helper function to abstract parsing a item from a dictionary or a list.\n\ns/a item/an item/\n\n> + * The differences of them in a YAML event stream are:\n> + *\n> + * 1. The start and end event type are different\n\ns/type/types/\n\n> + * 2. There is a leading scalar string as key in the items of a dictionary\n> + *\n> + * The caller should handle the leading key string in its callback parseItem\n> + * when it's a dictionary.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to initialize\n> + */\n> +int YamlParserContext::parseDictionaryOrList(bool isDictionary,\n> +\t\t\t\t\t const std::function &parseItem)\n> +{\n> +\tyaml_event_type_t endEventType = YAML_SEQUENCE_END_EVENT;\n> +\tif (isDictionary)\n> +\t\tendEventType = YAML_MAPPING_END_EVENT;\n> +\n> +\t/*\n> +\t * Add a safety counter to make sure we don't loop indefinitely in case\n> +\t * the configuration file is malformed.\n> +\t */\n> +\tunsigned int sentinel = 1000;\n> +\tdo {\n\n\tfor (unsigned int sentinel = 1000; sentinel; --sentinel) {\n\n> +\t\tauto evt = getEvent();\n> +\t\tif (!evt)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tif (evt->type == endEventType)\n> +\t\t\treturn 0;\n> +\n> +\t\tint ret = parseItem(std::move(evt));\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t--sentinel;\n\nand drop this.\n\n> +\t} while (sentinel);\n> +\n> +\tif (!sentinel)\n> +\t\treturn -EINVAL;\n\nYou can return -EINVAL unconditionally.\n\nI wonder if there's a need for more than 1000 items in a dictionary or\nlist. I could imagine that a list storing a LUT (for instance a lens\nshading correction table) could grow big. Maybe that will make parsing\nvery inefficient though, and we'll need a binary format instead at that\npoint ? For now I suppose the limit is fine, but please add a LOG(Error)\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParserContext::parseNextYamlObject()\n> + * \\brief Parse next YAML event and read it as a YamlObject\n> + * \\param[in] yamlObject The result of YamlObject\n> + * \\param[in] event The leading event of the object\n> + *\n> + * Parse next YAML object separately as a value, list or dictionary.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL Fail to parse the YAML file.\n> + */\n> +int YamlParserContext::parseNextYamlObject(YamlObject &yamlObject, EventPtr event)\n> +{\n> +\tif (!event)\n> +\t\treturn -EINVAL;\n> +\n> +\tswitch (event->type) {\n> +\tcase YAML_SCALAR_EVENT:\n> +\t\tyamlObject.type_ = YamlObject::Value;\n> +\t\treadValue(yamlObject.value_, std::move(event));\n> +\t\treturn 0;\n> +\n> +\tcase YAML_SEQUENCE_START_EVENT: {\n> +\t\tyamlObject.type_ = YamlObject::List;\n> +\t\tauto &list = yamlObject.list_;\n> +\t\tauto handler = [this, &list](EventPtr evt) {\n> +\t\t\tlist.emplace_back(new YamlObject());\n> +\t\t\treturn parseNextYamlObject(*list.back(), std::move(evt));\n> +\t\t};\n> +\t\treturn parseDictionaryOrList(false, handler);\n> +\t}\n> +\n> +\tcase YAML_MAPPING_START_EVENT: {\n> +\t\tyamlObject.type_ = YamlObject::Dictionary;\n> +\t\tauto &dictionary = yamlObject.dictionary_;\n> +\t\tauto handler = [this, &dictionary](EventPtr evtKey) {\n> +\t\t\t/* Parse key */\n> +\t\t\tif (evtKey->type != YAML_SCALAR_EVENT) {\n> +\t\t\t\tLOG(YamlParser, Error) << \"Expect key at line: \"\n> +\t\t\t\t\t\t << evtKey->start_mark.line\n> +\t\t\t\t\t\t << \" column: \"\n> +\t\t\t\t\t\t << evtKey->start_mark.column;\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tstd::string key;\n> +\t\t\treadValue(key, std::move(evtKey));\n> +\n> +\t\t\t/* Parse value */\n> +\t\t\tEventPtr evtValue = getEvent();\n> +\t\t\tif (!evtValue)\n> +\t\t\t\treturn -EINVAL;\n> +\n> +\t\t\tdictionary[key].reset(new YamlObject());\n> +\t\t\treturn parseNextYamlObject(*dictionary[key], std::move(evtValue));\n\nLet's avoid the double lookup:\n\n\t\t\tauto elem = dictionary.emplace(key, std::make_unique());\n\t\t\treturn parseNextYamlObject(elem.first->second.get(), std::move(evtValue));\n\n(untested)\n\n> +\t\t};\n> +\t\treturn parseDictionaryOrList(true, handler);\n> +\t}\n\nMissing blank line.\n\n> +\tdefault:\n> +\t\tLOG(YamlParser, Error) << \"Invalid YAML file\";\n\nYou can move the return -EINVAL here.\n\n> +\t}\n> +\n> +\treturn -EINVAL;\n> +}\n> +\n> +#endif /* __DOXYGEN__ */\n> +\n> +/**\n> + * \\class YamlParser\n> + * \\brief A helper class for parsing YAML file\n\ns/YAML file/a YAML file/\n\n> + *\n> + * The YamlParser class provides an easy interface to parse the contents of a\n> + * YAML file int a tree of YamlObject instances.\n\ns/int a/into a/\n\n> + *\n> + * Example usage:\n> + *\n> + * \\code{.unparsed}\n> + *\n> + * name:\n> + * \t\"John\"\n> + * numbers:\n> + * \t- 1\n> + * \t- 2\n> + *\n> + * \\endcode\n> + *\n> + * The following code illustrates how to parse the above YAML file:\n> + *\n> + * \\code{.cpp}\n> + *\n> + * std::unique_ptr root = YamlParser::parse(fh);\n> + * if (!root)\n> + * return;\n> + *\n> + * if (!root->isDictionary())\n> + * return;\n> + *\n> + * const YamlObject &name = (*root)[\"name\"];\n> + * std::cout << name.get(\"\");\n\n * std::cout << name.get(\"\") << std:endl();\n\n> + *\n> + * const YamlObject &numbers = (*root)[\"numbers\"];\n> + * if (!numbers.isList())\n> + * return;\n> + *\n> + * for (std::size_t i = 0; i < numbers.size(); i++)\n> + * std::cout << numbers[i].get(0);\n\nSame here.\n\n> + *\n> + * \\endcode\n> + *\n> + * The YamlParser::parse() function takes open FILE, parses its contents, and\n\ns/open/an open/\n\n> + * returns a pointer to a YamlObject corresponding to the root node of the YAML\n> + * document.\n> + */\n> +\n> +/**\n> + * \\fn YamlParser::parse()\n> + * \\brief Parse a YAML file as a YamlObject\n> + * \\param[in] fh The YAML file to parse\n> + *\n> + * The YamlParser::parse() function takes open FILE, parses its contents, and\n\ns/open/an open/\n\n> + * returns a pointer to a YamlObject corresponding to the root node of the YAML\n> + * document.\n\n\nAnd I'd add \"The caller is responsible for closing the file.\".\n\n> + *\n> + * \\return Pointer to result YamlObject on success or nullptr otherwise\n> + */\n> +std::unique_ptr YamlParser::parse(std::FILE *fh)\n> +{\n> +\tYamlParserContext context;\n> +\n> +\tif (context.init(fh))\n> +\t\treturn nullptr;\n> +\n> +\tstd::unique_ptr root(new YamlObject());\n> +\n> +\tif (context.parseContent(*root))\n\nPlease LOG() an error here.\n\nOverall this looks good, comments are minor. I expect to merge v6. Thank\nyou for your work !\n\n> +\t\treturn nullptr;\n> +\n> +\treturn root;\n> +}\n> +\n> +} /* namespace libcamera */","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 AF3DFC0F2A\n\tfor ;\n\tWed, 27 Apr 2022 01:12:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E176765644;\n\tWed, 27 Apr 2022 03:12:58 +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 E25D6604A6\n\tfor ;\n\tWed, 27 Apr 2022 03:12:56 +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 3DA5E30B;\n\tWed, 27 Apr 2022 03:12:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651021978;\n\tbh=zz504l9+XWQUFvsaXh7RmTJs+EGwjOcgp2TQMyF5JkM=;\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=gL2xswsKzzWCjWpTeNPok6ALhbnzCMTSN9zfStqlTMMT49kOBGxSx0/pg/qLgiALB\n\tswIAIZ3205lDEVx6ixhBQYWsAGQbMWjWkftwBKVSy42xIy7J8ZqQ8CzAoXytYQXAku\n\tnmgiuY3Bz1YGlPNtRkLtEASJfQ0ppl1xXfq1IRFZipTJSNmaEFUn/5Cffi9ilWjKUB\n\tZNkwXLCrhVBoh7NgLdebRunAHeJlLoyTowD+8QDnrEqI9ofy1MfxdPPPRI/3EPGx1v\n\tsuFDkE+hlVesWQPVBoJEqppEr+mcgomlnBegalkXawSfQSYEhmxUBQvIvhFgOKavfr\n\tnAA9D8NZQNSeQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651021976;\n\tbh=zz504l9+XWQUFvsaXh7RmTJs+EGwjOcgp2TQMyF5JkM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m78WoI+c9WPur+QkE+IL6DcLYcqyemvA8L2dCz9tdE5GctLKeDle+FArnWsgtSFXd\n\t9hP+DYtqkecTVj/+KE0FzLnRoKt2/QMOCe21EEDYQuo5LmoUsF+S/iZ21I1k90tDLJ\n\t/HsEWwXPJ/zXIaKFoDM2rhg4rt07PKwYYsGQwt2I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"m78WoI+c\"; dkim-atps=neutral","Date":"Wed, 27 Apr 2022 04:12:55 +0300","To":"Han-Lin Chen ","Message-ID":"","References":"<20220425144617.2549778-1-hanlinchen@chromium.org>\n\t<20220425144617.2549778-2-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220425144617.2549778-2-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce\n\tYamlParser as a helper to parse yaml files","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\" "}}]