[{"id":22595,"web_url":"https://patchwork.libcamera.org/comment/22595/","msgid":"","date":"2022-04-06T00:22:01","subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: Introduce YamlParser\n\tas 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 Wed, Feb 09, 2022 at 03:19:11PM +0800, Han-Lin Chen 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> \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 | 2 +-\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/yaml_parser.h | 82 +++\n> src/libcamera/meson.build | 3 +\n> src/libcamera/yaml_parser.cpp | 796 +++++++++++++++++++++++\n> 5 files changed, 883 insertions(+), 1 deletion(-)\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 ca8a97cb..4a2a451a 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\nYou can then drop libyaml-dev from the android section further down.\n\n> \n> for IPA module signing: [required]\n> libgnutls28-dev openssl\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..7ba7c52c\n> --- /dev/null\n> +++ b/include/libcamera/internal/yaml_parser.h\n> @@ -0,0 +1,82 @@\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\nNit-picking, I think yaml should be written YAML in\ncomments/documentation as it's an acronym. \"Yaml\" in class names is\nfine.\n\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n\nI really like how this fully encapsulates libyaml.\n\n> +namespace libcamera {\n> +\n> +class YamlObject\n> +{\n> +public:\n> +\tYamlObject() = default;\n> +\t~YamlObject() = default;\n\nYou may want to move the = default to the .cpp file, to avoid inlining\nconstructors and destructors.\n\n> +\n> +\tYamlObject(const YamlObject &) = delete;\n> +\tYamlObject &operator=(const YamlObject &) = delete;\n\nYou can use the LIBCAMERA_DISABLE_COPY() macro instead (there are a few\nexamples through libcamera).\n\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> +\tbool asBool(bool defaultValue = false, bool *ok = nullptr) const;\n> +\tdouble asDouble(double defaultValue = 0.0, bool *ok = nullptr) const;\n> +\tint32_t asInt32(int32_t defaultValue = 0, bool *ok = nullptr) const;\n> +\tuint32_t asUint32(uint32_t defaultValue = 0, bool *ok = nullptr) const;\n> +\tstd::string asString(std::string defaultValue = \"\", bool *ok = nullptr) const;\n> +\tSize asSize(Size defaultValue = Size(), bool *ok = nullptr) const;\n\nWould a template function we simpler ?\n\n\ttemplate\n\tconst T &get(const T &defaultValue, bool *ok = nullptr) const;\n\nIt could then be called as\n\n\tyaml.get(0);\n\nThat may prevent default values for the defaultValue argument though\n(unless = {} works), but maybe that's not a bad thing, specifying the\ndefault value explictly forces the caller to think about it.\n\n> +\n> +\tint length() const;\n\nTo make this more std::-like, how about renaming length to size ?\n\nThe function should return an unsigned int, as the size can't be\nnegative. We could make it std::size_t too.\n\n> +\tconst YamlObject &operator[](int index) const;\n\nSame here, the index should be unsigned, possible std::size_t (and the\nsame as the return type of the size() function).\n\n> +\n> +\tbool isMember(const std::string &key) const;\n\ns/isMember/hasMember/\n\nBut to make it more std::-like, I would rename this to contains(). The\nget() function could then be an overload of operator[] with a\nstd::string argument.\n\n> +\tconst YamlObject &get(const std::string &key) const;\n> +\tstd::vector getMemberNames() const;\n\nAs this is typically used to iterator over the dictionary entries, it\nwould be nice to implement begin() and end() instead, with an iterator.\nIt could be as simple as\n\n\tusing const_iterator = std::map>::iterator;\n\tconst_iterator begin() const { return dictionary_.cbegin(); }\n\tconst_iterator end() const { return dictionary_.cbegin(); }\n\nbut that would expose the unique_ptr and the non-const YamlObject. An\niterator that wraps the map iterator and returns a const\nstd::pair could be better.\n\nThis however only takes care of iteration over the dictionary, not the\nlist. I'll try to see if we could combine both, you don't have to\naddress it in the next version.\n\n> +\n> +private:\n> +\tfriend class YamlParser;\n> +\n> +\tenum PropertyType {\n\nMaybe just Type ? You could also make it an enum class, it's more\ntype-safe.\n\n> +\t\tDICTIONARY,\n> +\t\tLIST,\n> +\t\tVALUE,\n\nThese should use CamelCase.\n\n> +\t} type_;\n\nCould you split the type and variable declaration ? We don't usually\ncombine them.\n\n> +\n> +\tstd::string value_;\n> +\tstd::vector> list_;\n> +\tstd::map> dictionary_;\n> +};\n> +\n> +class YamlParser final : public Extensible\n> +{\n> +\tLIBCAMERA_DECLARE_PRIVATE()\n> +\n> +public:\n> +\tYamlParser();\n> +\tint ParseAsYamlObject(FILE *fh, YamlObject &yamlObject);\n\nFunction names should start with a lower case letter.\n\n> +\n> +private:\n> +\tint ParseNextYamlObject(YamlObject &yamlObject);\n\nSame here.\n\nIs there a reason this function is here and not in the Private class ?\nYou can make YamlParser::Private a friend of YamlObject if needed.\n\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..203d9cf4\n> --- /dev/null\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -0,0 +1,796 @@\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> +#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> +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 a yaml content. A\n\ns/a yaml content/yaml content/\n\n> + * YamlObject can be a dictionary or list of YamlObjects or a value if a tree\n> + * leaf.\n> + */\n> +\n> +/**\n> + * \\fn YamlObject::isValue()\n> + * \\brief Return whether the YamlObject is a value\n> + *\n> + * \\return Whether the YamlObject is a value\n\n * \\return True if the YammObject is a value, false otherwise\n\nto match the usual wording in the documentation. Same below.\n\n> + */\n> +\n> +/**\n> + * \\fn YamlObject::isList()\n> + * \\brief Return whether the YamlObject is a list\n> + *\n> + * \\return Whether the YamlObject is a list\n> + */\n> +\n> +/**\n> + * \\fn YamlObject::isDictionary()\n> + * \\brief Return whether the YamlObject is a dictionary\n> + *\n> + * \\return Whether the YamlObject is a dictionary\n> + */\n> +\n> +/**\n> + * \\fn YamlObject::asBool()\n> + * \\brief Helper function to parse the YamlObject as a bool value\n\ns/Helper function to parse/Parse/\n\nYou can drop \"helper function\" from other locations below.\n\n> + * \\param[in] defaultValue The default value when fail to parse\n\ns/when fail to parse/when failing to parse/ (or \"when parsing fails\")\n\nSame below.\n\n> + * \\param[in] ok The result of whether the parse success\n\ns/param[in]/param[out]/\ns/success/succeeded/\n\n> + *\n> + * Helper function to parse the YamlObject as a bool value.\n\nThere's no need to duplicate the \\brief, but extending it is useful:\n\n * This function parses the value of the YamlObject as a boolean, and returns\n * the value. If parsing fails (usually because the YamlObject doesn't store a\n * boolean value), the \\a defaultValue is returned, and \\a ok is set to false.\n * Otherwise, the YamlObject boolean value is returned, and \\a ok is set to\n * 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\nSame below.\n\n> + *\n> + * \\return Value as a bool type\n> + */\n> +bool YamlObject::asBool(bool defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != VALUE) {\n> +\t\treturn defaultValue;\n> +\t}\n\nNo need for curly braces.\n\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> +/**\n> + * \\fn YamlObject::asInt32()\n> + * \\brief Helper function to parse the YamlObject as a int32_t value\n> + * \\param[in] defaultValue The default value when fail to parse\n> + * \\param[in] ok The result of whether the parse success\n> + *\n> + * Helper function to parse the YamlObject as a int32_t value.\n> + *\n> + * \\return Value as a int32_t type\n> + */\n> +int32_t YamlObject::asInt32(int32_t defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != VALUE)\n> +\t\treturn defaultValue;\n> +\n> +\tint32_t value = std::strtol(value_.c_str(), nullptr, 10);\n> +\tif (errno == ERANGE)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::asUint32()\n> + * \\brief Helper function to parse the YamlObject as a uint32_t value\n> + * \\param[in] defaultValue The default value when fail to parse\n> + * \\param[in] ok The result of whether the parse success\n> + *\n> + * Helper function to parse the YamlObject as a uint32_t value.\n> + *\n> + * \\return Value as a uint32_t type\n> + */\n> +uint32_t YamlObject::asUint32(uint32_t defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n> +\n> +\tif (type_ != VALUE) {\n> +\t\treturn defaultValue;\n> +\t}\n\nNo need for curly braces.\n\n> +\n> +\tuint32_t value = std::strtoul(value_.c_str(), nullptr, 10);\n> +\tif (errno == ERANGE)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::asDouble()\n> + * \\brief Helper function to parse the YamlObject as a double value\n> + * \\param[in] defaultValue The default value when fail to parse\n> + * \\param[in] ok The result of whether the parse success\n> + *\n> + * Helper function to parse the YamlObject as a double value.\n> + *\n> + * \\return Value as a double type\n> + */\n> +double YamlObject::asDouble(double defaultValue, bool *ok) const\n> +{\n> +\tsetOk(ok, false);\n\nPlease add a blank line here, to match the other functions.\n\n> +\tif (type_ != VALUE)\n> +\t\treturn defaultValue;\n> +\n> +\tdouble value = std::strtod(value_.c_str(), nullptr);\n> +\tif (errno == ERANGE)\n> +\t\treturn defaultValue;\n> +\n> +\tsetOk(ok, true);\n> +\treturn value;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::asString()\n> + * \\brief Helper function to parse the YamlObject as a string value\n> + * \\param[in] defaultValue The default value when fail to parse\n> + * \\param[in] ok The result of whether the parse success\n> + *\n> + * Helper function to parse the YamlObject as a string value.\n> + *\n> + * \\return Value as a string type\n> + */\n> +std::string YamlObject::asString(std::string defaultValue, bool *ok) const\n> +{\nµ> +\tif (type_ != VALUE) {\n> +\t\tsetOk(ok, false);\n> +\t\treturn defaultValue;\n> +\t}\n> +\n\n*ok will not be set to true here. I'd use the same style as in the above\nfunctions.\n\n> +\treturn value_;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::asSize()\n> + * \\brief Helper function to parse the YamlObject as a Size value\n> + * \\param[in] defaultValue The default value when fail to parse\n> + * \\param[in] ok The result of whether the parse success\n> + *\n> + * Helper function to parse the YamlObject as a Size value. The Size structure\n> + * is represented as a list of two non-negative numbers.\n> + *\n> + * \\return Value as a Size type\n> + */\n> +Size YamlObject::asSize(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> +\tbool isInt = false;\n> +\tint width = list_[0]->asUint32(0, &isInt);\n\ns/int/unsigned int/\n\nYou could pass ok to the function instead of &isInt, and drop the isInt\nvariable.\n\n> +\tif (!isInt)\n> +\t\treturn Size(0, 0);\n> +\n> +\tint height = list_[1]->asUint32(0, &isInt);\n\nHere too.\n\n> +\tif (!isInt)\n> +\t\treturn Size(0, 0);\n> +\n> +\tsetOk(ok, true);\n> +\treturn Size(width, height);\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::length()\n> + * \\brief Helper function to return length of the list\n> + *\n> + * Helper function to parse the YamlObject as a List and return the length of\n> + * the List.\n> + *\n> + * \\return The length as a list\n\n(Assuming the function gets renamed to size() as discussed above)\n\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 * 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\nSimilarly below for the undefined behaviour description.\n\n> + */\n> +int YamlObject::length() const\n> +{\n> +\tassert(type_ == LIST);\n> +\n> +\treturn list_.size();\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::operator[](int index)\n> + * \\brief Helper function to return an element of a list YamlObject by index\n> + *\n> + * Helper function to return an element of a list YamlObject by index.\n> + *\n> + * \\return The YamlObject as an element of the list\n> + */\n> +const YamlObject &YamlObject::operator[](int index) const\n> +{\n> +\tassert(type_ == LIST);\n> +\n> +\treturn *list_[index];\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::isMember()\n> + * \\brief Helper function to check if an element of a dictionary exists\n> + *\n> + * Helper function to return an element of a list YamlObject by index.\n> + *\n> + * \\return Whether an element exists\n> + */\n> +bool YamlObject::isMember(const std::string &key) const\n> +{\n> +\tassert(type_ == DICTIONARY);\n> +\n> +\tif (dictionary_.find(key) == dictionary_.end())\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\fn YamlObject::getMemberNames()\n> + * \\brief Helper function to get all member names of the dictionary\n> + *\n> + * Helper function to get all member names of the dictionary.\n> + *\n> + * \\return A vector of string as the member names\n> + */\n> +std::vector YamlObject::getMemberNames() const\n> +{\n> +\tassert(type_ == DICTIONARY);\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::get()\n> + * \\brief Helper function to get a member by name from the dictionary\n> + *\n> + * Helper function to get a member by name from the dictionary\n> + *\n> + * \\return A YamlObject as a member\n> + */\n> +const YamlObject &YamlObject::get(const std::string &key) const\n> +{\n> +\tassert(type_ == DICTIONARY);\n> +\tassert(isMember(key));\n> +\n> +\tauto iter = dictionary_.find(key);\n> +\treturn *iter->second;\n> +}\n> +\n> +class YamlParser::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(YamlParser)\n> +\n> +public:\n> +\tPrivate();\n> +\t~Private();\n> +\n> +\tusing ItemParser = const std::function &;\n> +\tusing RecordParser = const std::function &;\n> +\n> +\tint initParser(FILE *fh);\n> +\tvoid release();\n> +\n> +\tint consumeDocumentStart();\n> +\tint consumeDocumentEnd();\n> +\n> +\tint parseString(std::string &value);\n> +\tint parseList(ItemParser parseItem);\n> +\tint parseDictionary(RecordParser parseRecord);\n> +\n> +private:\n> +\tint nextEvent(yaml_event_t &event);\n> +\tint consume(yaml_event_type_t);\n> +\n> +\tint parseDictionaryOrList(bool isDictionary,\n> +\t\t\t\t const std::function &parseItem);\n> +\n> +\tbool nextEventLoaded_;\n> +\tyaml_event_t nextEvent_;\n> +\n> +\tbool parserValid_;\n> +\tyaml_parser_t parser_;\n> +};\n> +\n> +/**\n> + * \\class YamlParser::Private\n> + * \\brief A Private class helps control event based parsing for yaml files.\n\nMaybe\n\n * \\brief Base class for YamlParser private data\n\nto match the documentation of other private classes ?\n\n> + *\n> + * The YamlParser::Private class stores the internal yaml_parser_t and provides\n> + * helper functions to do event based parsing for yaml files.\n\ns/event base/event-based/\ns/yaml/YAML/\n\n> + */\n> +YamlParser::Private::Private()\n> +\t: nextEventLoaded_(false), parserValid_(false)\n> +{\n> +}\n> +\n> +/**\n> + * \\class YamlParser::Private\n> + * \\brief Destructor of YamlParser::Private.\n> + */\n> +YamlParser::Private::~Private()\n> +{\n> +\trelease();\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::initParser()\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 YamlParser must be initialized with\n> + * an opened FILE to create an internal parser. The FILE need to stay valid\n> + * during the process. The release() should be called after use.\n\n The release() function must be called after parsing completes.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to initialize\n\ns/is failed/failed/ (of \"has failed\")\n\n> + */\n> +int YamlParser::Private::initParser(FILE *fh)\n> +{\n> +\t/* yaml_parser_initialize returns 1 when succeeded */\n\ns/when succeeded/on success/ (or \"when it succeeds\")\n\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 YamlParser::Private::release()\n> + * \\brief Release the internal parser\n> + *\n> + * After parsing the yaml content, The YamlParser::release() should be called\n> + * to release the internal parser.\n> + */\n> +void YamlParser::Private::release()\n> +{\n> +\tif (nextEventLoaded_) {\n> +\t\tyaml_event_delete(&nextEvent_);\n> +\t\tnextEventLoaded_ = false;\n> +\t}\n> +\n> +\tif (parserValid_) {\n> +\t\tparserValid_ = false;\n> +\t\tyaml_parser_delete(&parser_);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::consume()\n> + * \\brief Cosume the next event with an expected event type\n\ns/Cosume/Consume/\n\n> + *\n> + * \\param[in] type The expected event type to consume\n> + *\n> + * Consume next event and check whether next event has an expected type.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to initialize\n> + */\n> +int YamlParser::Private::consume(yaml_event_type_t type)\n> +{\n> +\tyaml_event_t event;\n> +\tint ret = nextEvent(event);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (event.type != type) {\n> +\t\tLOG(YamlParser, Error)\n> +\t\t\t<< \"Expect event: \" << type\n> +\t\t\t<< \" but get \" << event.type\n> +\t\t\t<< \" at line: \" << event.start_mark.line\n> +\t\t\t<< \" column: \" << event.start_mark.column;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tyaml_event_delete(&event);\n> +\tnextEventLoaded_ = false;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\typedef YamlParser::Private::ItemParser\n> + * \\brief The functor to handle an item in a yaml list\n> + */\n> +\n> +/**\n> + * \\typedef YamlParser::Private::RecordParser\n> + * \\brief The functor to handle an item in a yaml dictionary\n> + */\n> +\n> +/**\n> + * \\fn YamlParser::Private::consumeDocumentStart()\n> + * \\brief Consume start of a yaml document\n> + *\n> + * Check yaml start of a yaml document. The function should be called and\n> + * checked before parsing any content of the yaml file.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to validate start of a yaml file\n> + */\n> +int YamlParser::Private::consumeDocumentStart()\n> +{\n> +\tif (consume(YAML_STREAM_START_EVENT))\n> +\t\treturn -EINVAL;\n> +\n> +\tif (consume(YAML_DOCUMENT_START_EVENT))\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::consumeDocumentEnd()\n> + * \\brief Consume end of a yaml document\n> + *\n> + * Check yaml end of a yaml document. The function should be called and\n> + * checked after parsing any content of the yaml file.\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> + */\n> +int YamlParser::Private::consumeDocumentEnd()\n> +{\n> +\tif (consume(YAML_DOCUMENT_END_EVENT))\n> +\t\treturn -EINVAL;\n> +\n> +\tif (consume(YAML_STREAM_END_EVENT))\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::parseString()\n> + * \\brief Parse scalar and read its content as a string\n> + * \\param[in] value The string reference to fill value\n> + *\n> + * A helper function to peek the next event, check whether it's scalar type\n> + * and read its content as a string.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to initialize\n> + */\n> +int YamlParser::Private::parseString(std::string &value)\n> +{\n> +\tyaml_event_t event;\n> +\tint ret = nextEvent(event);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (event.type != YAML_SCALAR_EVENT) {\n> +\t\tLOG(YamlParser, Error) << \"Expect scalar at line: \"\n> +\t\t\t\t << event.start_mark.line\n> +\t\t\t\t << \" column: \"\n> +\t\t\t\t << event.start_mark.column;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tvalue.assign(reinterpret_cast(event.data.scalar.value),\n> +\t\t event.data.scalar.length);\n> +\n> +\tconsume(YAML_SCALAR_EVENT);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::parseList()\n> + * \\brief Parse a list with an callback function to parse each item in the list\n> + * \\param[in] parseItem The functor to parse a single item in the list\n> + *\n> + * Start to parse a list from the current position and call back to parseItem\n> + * for parsing each item in the list. The parseItem should return 0 on success\n> + * or a negative error code otherwise.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to parse the list\n> + */\n> +int YamlParser::Private::parseList(ItemParser parseItem)\n> +{\n> +\treturn parseDictionaryOrList(false, parseItem);\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::parseDictionary()\n> + * \\brief Parse a dictionary with an callback function to parse each item\n> + * \\param[in] parseRecord The functor to parse a single item in the dictionary\n> + *\n> + * Start to parse a dictionary from the current position and call back to\n> + * parseRecord to parse value by a string argument as the key to the value.\n> + * The parseRecord should return 0 on success or a negative error code\n> + * otherwise.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to parse the dictionary\n> + */\n> +int YamlParser::Private::parseDictionary(RecordParser parseRecord)\n> +{\n> +\tauto parseItem = [this, &parseRecord]() {\n> +\t\tstd::string key;\n> +\t\tint ret = parseString(key);\n> +\t\tif (ret)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\treturn parseRecord(key);\n> +\t};\n> +\n> +\treturn parseDictionaryOrList(true, parseItem);\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::parseDictionaryOrList()\n> + * \\brief A helper function to abstract common part of parsing dictionary or list\n> + *\n> + * \\param[in] isDictionary True for parsing a dictionary, and false for a list\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> + * The differences of them in a yaml event stream are:\n> + *\n> + * 1. The start and end event type are different\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 YamlParser::Private::parseDictionaryOrList(bool isDictionary,\n> +\t\t\t\t\t const std::function &parseItem)\n> +{\n> +\tyaml_event_type_t startEventType = YAML_SEQUENCE_START_EVENT;\n> +\tyaml_event_type_t endEventType = YAML_SEQUENCE_END_EVENT;\n> +\n> +\tif (isDictionary) {\n> +\t\tstartEventType = YAML_MAPPING_START_EVENT;\n> +\t\tendEventType = YAML_MAPPING_END_EVENT;\n> +\t}\n> +\n> +\tif (consume(startEventType))\n> +\t\treturn -EINVAL;\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> +\tyaml_event_t event;\n> +\tdo {\n> +\t\tint ret = nextEvent(event);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tif (event.type == endEventType)\n> +\t\t\treturn consume(endEventType);\n> +\n> +\t\tret = parseItem();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t--sentinel;\n> +\t} while (sentinel);\n> +\n> +\tif (!sentinel)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::Private::nextEvent()\n> + * \\brief Peek the next event\n> + *\n> + * \\param[in] event The event reference to fill information\n> + *\n> + * Peek the next event in the current yaml event stream, and return -EINVAL when\n> + * there is no more event.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The parser is failed to initialize\n> + */\n> +int YamlParser::Private::nextEvent(yaml_event_t &event)\n> +{\n> +\tif (nextEventLoaded_) {\n> +\t\tevent = nextEvent_;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* yaml_parser_parse returns 1 when succeeded */\n> +\tif (1 != yaml_parser_parse(&parser_, &nextEvent_)) {\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tnextEventLoaded_ = true;\n> +\tevent = nextEvent_;\n> +\n> +\treturn 0;\n> +}\n\nI'll review the internals of the parser in details on the next\niteration, as there are enough review comments already :-)\n\n> +\n> +/**\n> + * \\class YamlParser\n> + * \\brief A helper class for parsing yaml files.\n\ns/yaml files./YAML files/\n\n> + *\n> + * The YamlParser class eases handling of parsing a yaml file by providing\n> + * helper function to extract content yaml files into a tree base yaml object.\n\n * The YamlParser class provides an easy interface to parse the contents of a\n * YAML file into a tree of YamlObject instances.\n\n> + *\n> + * Example usage 1:\n> + * The following code illustrates how to parse the following yaml file:\n> + *\n> + * name:\n> + * \t\"John\"\n> + * numbers:\n> + * \t- 1\n> + * \t- 2\n> + *\n> + * @code\n\nThat should be \\code to match the style of the other doxygen tags. Same\nfor endcode.\n\n> + *\n> + *\tYamlObject root;\n> + *\tYamlParser yamlParser;\n> + *\tif (yamlParser.ParseAsYamlObject(fh, root));\n\nExtra ; at the end of the line.\n\n> + *\t\treturn;\n> + *\n> + *\tif (!root.isDictionary())\n> + *\t\treturn;\n> + *\n> + *\tstd::string name = root.get(\"name\");\n> + *\tcout << name.asString();\n\n *\tstd::cout << name.asString() << std::endl;\n\n> + *\n> + *\tconst YamlObject &numbers = root.get(\"numbers\");\n> + *\tif (!numbers.isList())\n> + *\t\treturn;\n> + *\n> + *\tfor (int i = 0; i < numbers.size; i++)\n> + *\t\tcout << numbers[i].asInt();\n\n *\t\tstd::cout << numbers[i].asInt() << std::endl;\n\nIt would be nice to be able to write\n\n *\tfor (const YamlObject &number : numbers)\n *\t\tstd::cout << number.asInt() << std::endl;\n\n> + *\n> + * @endcode\n> + *\n> + * Function ParseAsYamlObject(FILE *, YamlObject &) accept an opened FILE and\n> + * initialize an internal parser. The FILE need to stay valid during function\n> + * call.\n\n * The ParseAsYamlObject() function accepts an open FILE and initializes an\n * internal parse.\n\nThe second sentence can be dropped, as there's no way for the caller to\nclose the file in the middle of the call :-)\n\n> + */\n> +\n> +/**\n> + * \\brief Construct a YamlParser\n> + */\n> +YamlParser::YamlParser()\n> +\t: Extensible(std::make_unique())\n> +{\n> +}\n> +\n> +/**\n> + * \\fn YamlParser::ParseAsYamlObject()\n\nAs we can't parse it as anything else than a YamlObject, I think I'd\ncall this function just parse().\n\n> + * \\brief Parse a yaml file as a YamlObject\n> + * \\param[in] fh The yaml file to parse\n> + * \\param[in] yamlObject The result fo YamlObject\n\nWouldn't it be simpler for the caller if this function returned the\nYamlObject instance ? The only error code we return is -EINVAL so\nthere's no need to differentiate between errors, \n\n> + *\n> + * Prior to parsing the yaml content, the function accepts an opened FILE to\n> + * create an internal parser. The FILE need to stay valid during the function\n> + * call. When fails, the contect of the YamlObject is undefined.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL Fail to parse the yaml file.\n> + */\n> +int YamlParser::ParseAsYamlObject(FILE *fh, YamlObject &yamlObject)\n> +{\n> +\tif (_d()->initParser(fh))\n> +\t\treturn -EINVAL;\n> +\n> +\tif (_d()->consumeDocumentStart())\n> +\t\tgoto error;\n> +\n> +\tif (ParseNextYamlObject(yamlObject))\n> +\t\tgoto error;\n> +\n> +\tif (_d()->consumeDocumentEnd())\n> +\t\tgoto error;\n> +\n> +\t_d()->release();\n> +\treturn 0;\n> +\n> +error:\n> +\t_d()->release();\n> +\treturn -EINVAL;\n> +}\n\nI'm tempted to make this function static, drop inheritance from\nExtensible, and rename YamlParser::Private to YamlParserContext.\n\n> +\n> +/**\n> + * \\fn YamlParser::ParseNextEvent()\n> + * \\brief Helper function to parse next yaml event and read it as a YamlObject\n> + * \\param[in] yamlObject The result of YamlObject\n> + *\n> + * A helper function to parse next yaml event by peeking next event and parse\n> + * them 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 YamlParser::ParseNextYamlObject(YamlObject &yamlObject)\n> +{\n> +\tyaml_event_t event;\n> +\n> +\tif (_d()->nextEvent(event))\n> +\t\treturn -EINVAL;\n> +\n> +\tif (event.type == YAML_SCALAR_EVENT) {\n> +\t\tyamlObject.type_ = YamlObject::VALUE;\n> +\t\t_d()->parseString(yamlObject.value_);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (event.type == YAML_SEQUENCE_START_EVENT) {\n> +\t\tyamlObject.type_ = YamlObject::LIST;\n> +\t\tauto &list = yamlObject.list_;\n> +\t\tauto handler = [this, &list]() {\n> +\t\t\tlist.emplace_back(new YamlObject());\n> +\t\t\treturn ParseNextYamlObject(*list.back());\n> +\t\t};\n> +\t\treturn _d()->parseList(handler);\n> +\t}\n> +\n> +\tif (event.type == YAML_MAPPING_START_EVENT) {\n> +\t\tyamlObject.type_ = YamlObject::DICTIONARY;\n> +\t\tauto &dictionary = yamlObject.dictionary_;\n> +\t\tauto handler = [this, &dictionary](const std::string &key) {\n> +\t\t\tdictionary[key].reset(new YamlObject());\n> +\t\t\treturn ParseNextYamlObject(*dictionary[key]);\n> +\t\t};\n> +\t\treturn _d()->parseDictionary(handler);\n> +\t}\n> +\n> +\tLOG(YamlParser, Error) << \"Invalid yaml file\";\n> +\treturn -EINVAL;\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 C7AF1C3256\n\tfor ;\n\tWed, 6 Apr 2022 00:22:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28C7865642;\n\tWed, 6 Apr 2022 02:22:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0187C633A5\n\tfor ;\n\tWed, 6 Apr 2022 02:22:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D0C6482;\n\tWed, 6 Apr 2022 02:22:04 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649204526;\n\tbh=HsS4loNUSII9bR6yPFm/1pAocJ/7zigkJH64vbRbcrY=;\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=be7UCR+EZZMH2qf2uu+smD0fnGddCQHa3NRp5DHnEdNXHubLkqUaprV5wu32Wf0Ts\n\tA/0rGHEG/NjW9tdvb5067NOmCzfCUlA1+zfdd+nN8ep2GCEn/wK3PEJAKkKqQkawf2\n\tCfXX61ZgqUn8Tr7jHOIGqdGjmUwj6E1JQ8hnr6HG4CzNBLOVeG9UK8MrmudFZIlj0Q\n\txInrHMn8LY5LEeMRI1NkQeXDfXYZ00uuHZGC4cmvlLXxsbzn2buVBxOTX9aZWn+KHn\n\tTe7xA5u0eInF31tqfvg8tJETZBzW1Uv60zi/oD8Ycq3CU4an4E77/Mkd6uHdiyJ7h0\n\tGM5C+JHLcEJFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649204524;\n\tbh=HsS4loNUSII9bR6yPFm/1pAocJ/7zigkJH64vbRbcrY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZxuVKbrxva9PIkxQwxddY9U7xdw75Lk2h8yvPBlT/HTI2x5QWgWA/mGoS26uDV0z9\n\t/4gKFsewX8eRB/HjItV28Y1ntCh3E+L7nZJhBiEnK3DVq+0sOzpmDcFJ5J+d20pFbp\n\tKKD2Pt9zqX0orowN4y9GdAaf+OPyi5ZZzc9UMGl8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZxuVKbrx\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 03:22:01 +0300","To":"Han-Lin Chen ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-4-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220209071917.559993-4-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: Introduce YamlParser\n\tas 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\" "}}]