[{"id":23390,"web_url":"https://patchwork.libcamera.org/comment/23390/","msgid":"<20220613041208.GE2369877@pyrite.rasen.tech>","date":"2022-06-13T04:12:08","subject":"Re: [libcamera-devel] [RFC PATCH v2 03/14] libcamera: yaml_parser:\n\tSwitch from FILE to File","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Laurent,\n\nOn Sat, Jun 04, 2022 at 09:59:28PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> THe FILE object isn't very user-friendly as it requires manual close.\n> Replace it with File to provide RAII-style resource management in the\n> YamlParser API.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/internal/yaml_parser.h | 4 +--\n> src/android/camera_hal_config.cpp | 16 +++++-----\n> src/libcamera/yaml_parser.cpp | 39 +++++++++++++++++-------\n> test/yaml-parser.cpp | 18 +++++------\n> 4 files changed, 47 insertions(+), 30 deletions(-)\n> \n> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h\n> index b4f852b1ce54..be5f0914703f 100644\n> --- a/include/libcamera/internal/yaml_parser.h\n> +++ b/include/libcamera/internal/yaml_parser.h\n> @@ -7,7 +7,6 @@\n> \n> #pragma once\n> \n> -#include \n> #include \n> #include \n> #include \n> @@ -18,6 +17,7 @@\n> \n> namespace libcamera {\n> \n> +class File;\n> class YamlParserContext;\n> \n> class YamlObject\n> @@ -82,7 +82,7 @@ private:\n> class YamlParser final\n> {\n> public:\n> -\tstatic std::unique_ptr parse(std::FILE *fh);\n> +\tstatic std::unique_ptr parse(File &file);\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> index 8ba8738cc6b6..ac484b8df1bd 100644\n> --- a/src/android/camera_hal_config.cpp\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/yaml_parser.h\"\n> @@ -27,7 +28,7 @@ class CameraHalConfig::Private : public Extensible::Private\n> public:\n> \tPrivate();\n> \n> -\tint parseConfigFile(FILE *fh, std::map *cameras);\n> +\tint parseConfigFile(File &file, std::map *cameras);\n> \n> private:\n> \tint parseCameraConfigData(const std::string &cameraId, const YamlObject &);\n> @@ -41,7 +42,7 @@ CameraHalConfig::Private::Private()\n> {\n> }\n> \n> -int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> +int CameraHalConfig::Private::parseConfigFile(File &file,\n> \t\t\t\t\t std::map *cameras)\n> {\n> \t/*\n> @@ -65,7 +66,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> \n> \tcameras_ = cameras;\n> \n> -\tstd::unique_ptr root = YamlParser::parse(fh);\n> +\tstd::unique_ptr root = YamlParser::parse(file);\n> \tif (!root)\n> \t\treturn -EINVAL;\n> \n> @@ -169,9 +170,9 @@ int CameraHalConfig::parseConfigurationFile()\n> \t\treturn -ENOENT;\n> \t}\n> \n> -\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> -\tif (!fh) {\n> -\t\tint ret = -errno;\n> +\tFile file(filePath);\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> +\t\tint ret = file.error();\n> \t\tLOG(HALConfig, Error) << \"Failed to open configuration file \"\n> \t\t\t\t << filePath << \": \" << strerror(-ret);\n> \t\treturn ret;\n> @@ -179,8 +180,7 @@ int CameraHalConfig::parseConfigurationFile()\n> \n> \texists_ = true;\n> \n> -\tint ret = _d()->parseConfigFile(fh, &cameras_);\n> -\tfclose(fh);\n> +\tint ret = _d()->parseConfigFile(file, &cameras_);\n> \tif (ret)\n> \t\treturn -EINVAL;\n> \n> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> index 5b872dbb0a2d..85f6694f5fde 100644\n> --- a/src/libcamera/yaml_parser.cpp\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -345,7 +346,7 @@ public:\n> \tYamlParserContext();\n> \t~YamlParserContext();\n> \n> -\tint init(std::FILE *fh);\n> +\tint init(File &file);\n> \tint parseContent(YamlObject &yamlObject);\n> \n> private:\n> @@ -358,6 +359,9 @@ private:\n> \t};\n> \tusing EventPtr = std::unique_ptr;\n> \n> +\tstatic int yamlRead(void *data, unsigned char *buffer, size_t size,\n> +\t\t\t size_t *sizeRead);\n> +\n> \tEventPtr nextEvent();\n> \n> \tvoid readValue(std::string &value, EventPtr event);\n> @@ -399,13 +403,13 @@ YamlParserContext::~YamlParserContext()\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> + * with a file to create an internal parser. The file needs to stay valid until\n> + * parsing completes.\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> +int YamlParserContext::init(File &file)\n> {\n> \t/* yaml_parser_initialize returns 1 when it succeededs */\n> \tif (!yaml_parser_initialize(&parser_)) {\n> @@ -413,11 +417,25 @@ int YamlParserContext::init(std::FILE *fh)\n> \t\treturn -EINVAL;\n> \t}\n> \tparserValid_ = true;\n> -\tyaml_parser_set_input_file(&parser_, fh);\n> +\tyaml_parser_set_input(&parser_, &YamlParserContext::yamlRead, &file);\n> \n> \treturn 0;\n> }\n> \n> +int YamlParserContext::yamlRead(void *data, unsigned char *buffer, size_t size,\n> +\t\t\t\tsize_t *sizeRead)\n> +{\n> +\tFile *file = static_cast(data);\n> +\n> +\tSpan buf{ buffer, size };\n> +\tssize_t ret = file->read(buf);\n> +\tif (ret < 0)\n> +\t\treturn 0;\n> +\n> +\t*sizeRead = ret;\n> +\treturn 1;\n> +}\n> +\n> /**\n> * \\fn YamlParserContext::nextEvent()\n> * \\brief Get the next event\n> @@ -655,21 +673,20 @@ int YamlParserContext::parseNextYamlObject(YamlObject &yamlObject, EventPtr even\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> + * \\param[in] file The YAML file to parse\n> *\n> - * The YamlParser::parse() function takes an open FILE, parses its contents, and\n> + * The YamlParser::parse() function takes a file, parses its contents, and\n> * returns a pointer to a YamlObject corresponding to the root node of the YAML\n> - * document. The caller is responsible for closing the file.\n> + * document.\n> *\n> * \\return Pointer to result YamlObject on success or nullptr otherwise\n> */\n> -std::unique_ptr YamlParser::parse(std::FILE *fh)\n> +std::unique_ptr YamlParser::parse(File &file)\n> {\n> \tYamlParserContext context;\n> \n> -\tif (context.init(fh))\n> +\tif (context.init(file))\n> \t\treturn nullptr;\n> \n> \tstd::unique_ptr root(new YamlObject());\n> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp\n> index c5b4ddbb19e5..e75f8fe8f642 100644\n> --- a/test/yaml-parser.cpp\n> +++ b/test/yaml-parser.cpp\n> @@ -9,6 +9,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"test.h\"\n> @@ -69,29 +71,27 @@ protected:\n> \tint run()\n> \t{\n> \t\t/* Test invalid YAML file */\n> -\t\tFILE *fh = fopen(invalidYamlFile_.c_str(), \"r\");\n> -\t\tif (!fh) {\n> +\t\tFile file{ invalidYamlFile_ };\n> +\t\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> \t\t\tcerr << \"Fail to open invalid YAML file\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tstd::unique_ptr root = YamlParser::parse(fh);\n> -\t\tfclose(fh);\n> -\n> +\t\tstd::unique_ptr root = YamlParser::parse(file);\n> \t\tif (root) {\n> \t\t\tcerr << \"Invalid YAML file parse successfully\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> \t\t/* Test YAML file */\n> -\t\tfh = fopen(testYamlFile_.c_str(), \"r\");\n> -\t\tif (!fh) {\n> +\t\tfile.close();\n> +\t\tfile.setFileName(testYamlFile_);\n> +\t\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> \t\t\tcerr << \"Fail to open test YAML file\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\troot = YamlParser::parse(fh);\n> -\t\tfclose(fh);\n> +\t\troot = YamlParser::parse(file);\n> \n> \t\tif (!root) {\n> \t\t\tcerr << \"Fail to parse test YAML file: \" << std::endl;\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4F6B0BD161\n\tfor ;\n\tMon, 13 Jun 2022 04:12:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEDDF65636;\n\tMon, 13 Jun 2022 06:12:17 +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 9AFDF600F0\n\tfor ;\n\tMon, 13 Jun 2022 06:12:16 +0200 (CEST)","from pyrite.rasen.tech (softbank036240126034.bbtec.net\n\t[36.240.126.34])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A19A440;\n\tMon, 13 Jun 2022 06:12:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655093538;\n\tbh=U8ARoD1MZN4ttEHwWj4rFzR+hMHanziMBbEYE0kVq8I=;\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=ZY+VmvXbNOCj/R5klMziFs5VNd2A+x9ZpYifxH/8+rzPvBuPVi4et08XRIG+2ulGB\n\tQTphBXOdHto7SppPilB3IBZeh7IdjPdX6Ql8H41/M37vg2uZNVQNJxzjHRTaH2y05P\n\twl8BdCsHQOFZ82Amwg5wLyrKoYCjcELaGp+weke7XQ9SC/wP90yhRA0yXPyhQPRtIB\n\tFw9Nwh9Ia1ea2+jUQW5RkQLtZj243n2BIBAvHUV4auRdAYTn6gEN5mlWf+7qZk4v+9\n\tOwxYFoWhWSeCXvTV5Q+AFeEgAk/LJIW0fyCy5STXRe1T8C3G0i6G4GS2lBV4Wo0I9A\n\tDyna0ESYgUPNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655093536;\n\tbh=U8ARoD1MZN4ttEHwWj4rFzR+hMHanziMBbEYE0kVq8I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y046W0ZzVL9gX4Pfw7OeKe1aF5HwIb3kfdS4sHoWjOinWpsto+IhCNMdiQsVueN4o\n\tBb3/UthlyCtqU5jrEX2dyGe3Cu+TC33fJL2WSHBx9ap5TFxSUddH5x/kFBnBsfFjVo\n\t8ha4Eu8NdSLsEN1bmvsjH/nINKAEUMAOJqWgs9ZM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Y046W0Zz\"; dkim-atps=neutral","Date":"Mon, 13 Jun 2022 13:12:08 +0900","To":"Laurent Pinchart ","Message-ID":"<20220613041208.GE2369877@pyrite.rasen.tech>","References":"<20220604185939.29163-1-laurent.pinchart@ideasonboard.com>\n\t<20220604185939.29163-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220604185939.29163-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 03/14] libcamera: yaml_parser:\n\tSwitch from FILE to File","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]