[{"id":34695,"web_url":"https://patchwork.libcamera.org/comment/34695/","msgid":"","date":"2025-06-27T14:00:44","subject":"Re: [PATCH v11 03/12] config: Add configuration retrieval helpers","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:\n> Let's add some helpers to make accessing simple configuration values\n> simpler.\n> \n> GlobalConfiguration::option ensures that no value is returned rather\n> than a value of YamlObject::empty.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> .../libcamera/internal/global_configuration.h | 5 ++\n> src/libcamera/global_configuration.cpp | 52 +++++++++++++++++++\n> 2 files changed, 57 insertions(+)\n> \n> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n> index 89984c84d..357119628 100644\n> --- a/include/libcamera/internal/global_configuration.h\n> +++ b/include/libcamera/internal/global_configuration.h\n> @@ -8,6 +8,8 @@\n> #pragma once\n> \n> #include \n> +#include \n> +#include \n> \n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> @@ -22,6 +24,9 @@ public:\n> \n> \tunsigned int version() const;\n> \tConfiguration configuration() const;\n> +\tstd::optional option(const std::string &confPath) const;\n> +\tstd::optional envOption(const char *const envVariable,\n> +\t\t\t\t\t const std::string &confPath) const;\n> \n> private:\n> \tbool loadFile(const std::filesystem::path &fileName);\n> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp\n> index 72b4a5246..068ddd5a5 100644\n> --- a/src/libcamera/global_configuration.cpp\n> +++ b/src/libcamera/global_configuration.cpp\n> @@ -8,6 +8,8 @@\n> #include \"libcamera/internal/global_configuration.h\"\n> \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> \n> @@ -39,6 +41,11 @@ LOG_DEFINE_CATEGORY(Configuration)\n> * is not found then in system-wide configuration directories. If multiple\n> * configuration files exist then only the first one found is used and no\n> * configuration merging is performed.\n> + *\n> + * The configuration can be accessed using the provided helpers. Namely\n> + * GlobalConfiguration::option() or GlobalConfiguration::envOption() to access\n> + * individual options or GlobalConfiguration::configuration() to access the\n> + * whole configuration.\n> */\n> \n> bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)\n> @@ -108,6 +115,51 @@ GlobalConfiguration::GlobalConfiguration()\n> * the underlying type.\n> */\n> \n> +/**\n> + * \\brief Return value of the configuration option identified by \\a confPath\n> + * \\param[in] confPath Sequence of the YAML section names (excluding\n> + * `configuration') leading to the requested option separated by slashes\n> + * \\return A value if an item corresponding to \\a confPath exists in the\n> + * configuration file, no value otherwise\n> + */\n> +std::optional GlobalConfiguration::option(const std::string &confPath) const\n\nI think we should avoid in-band signalling, so I would prefer removing the separators from the\nstring. And instead making the function take an `std::initializer_list` (and\n`Span` if an init list not sufficient, but I think it should be).\nI believe it is inevitable that someone will forget to consider if the input is \"safe\",\nespecially if not all path components are fixed.\n\n\n> +{\n> +\tconst YamlObject *c = &configuration();\n> +\tfor (auto part : utils::split(confPath, \"/\")) {\n> +\t\tc = &(*c)[part];\n> +\t\tif (!*c)\n> +\t\t\treturn {};\n> +\t}\n> +\treturn c->get();\n> +}\n> +\n> +/**\n> + * \\brief Return value of the configuration option from a file or environment\n> + * \\param[in] envVariable Environment variable to get the value from\n> + * \\param[in] confPath The same as in GlobalConfiguration::option\n> + *\n> + * This helper looks first at the given environment variable and if it is\n> + * defined then it returns its value (even if it is empty). Otherwise it looks\n> + * for \\a confPath the same way as in GlobalConfiguration::option. Only string\n> + * values are supported.\n> + *\n> + * \\note Support for using environment variables to configure libcamera behavior\n> + * is provided here mostly for backward compatibility reasons. Introducing new\n> + * configuration environment variables is discouraged.\n> + *\n> + * \\return A value retrieved from the given environment option or configuration\n> + * file or no value if not found\n> + */\n> +std::optional GlobalConfiguration::envOption(\n> +\tconst char *const envVariable,\n\nCould you please drop the top-level `const`?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +\tconst std::string &confPath) const\n> +{\n> +\tconst char *envValue = utils::secure_getenv(envVariable);\n> +\tif (envValue)\n> +\t\treturn std::optional{ std::string{ envValue } };\n> +\treturn option(confPath);\n> +}\n> +\n> /**\n> * \\brief Return configuration version\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 4787CC3237\n\tfor ;\n\tFri, 27 Jun 2025 14:00:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBCDB68E0B;\n\tFri, 27 Jun 2025 16:00:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEFEC68DFC\n\tfor ;\n\tFri, 27 Jun 2025 16:00:49 +0200 (CEST)","from [192.168.33.12] (185.221.143.107.nat.pool.zt.hu\n\t[185.221.143.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 44BFAFDB;\n\tFri, 27 Jun 2025 16:00:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oNHAwmRU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751032830;\n\tbh=kFcYDW1uKZimyPMYJkaouHQYVESdU6b4CVkZOeCZEGo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=oNHAwmRUd2X3D1mNg9w5cHrfP7V/XtEBkYX8yuq3MjJIM28UvY2hfq0BHPUqj/8R/\n\th1BeckwNcNmeU4uLbOqUn7b8E6xaH81iq7bJlhivzGMJS4VPK5Nus9+YUJTz2Zo4Ja\n\t+A6QaG/GUu+VYs1VsjGmLXhaS4UaMZiHsJola4uE=","Message-ID":"","Date":"Fri, 27 Jun 2025 16:00:44 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v11 03/12] config: Add configuration retrieval helpers","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"Kieran Bingham ,\n\tLaurent Pinchart ","References":"<20250624083612.27230-1-mzamazal@redhat.com>\n\t<20250624083612.27230-4-mzamazal@redhat.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250624083612.27230-4-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34711,"web_url":"https://patchwork.libcamera.org/comment/34711/","msgid":"<85frflvwps.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-06-27T18:57:35","subject":"Re: [PATCH v11 03/12] config: Add configuration retrieval helpers","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Barnabás,\n\nthank you for review.\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:\n>> Let's add some helpers to make accessing simple configuration values\n>> simpler.\n>> GlobalConfiguration::option ensures that no value is returned rather\n>> than a value of YamlObject::empty.\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../libcamera/internal/global_configuration.h | 5 ++\n>> src/libcamera/global_configuration.cpp | 52 +++++++++++++++++++\n>> 2 files changed, 57 insertions(+)\n>> diff --git a/include/libcamera/internal/global_configuration.h\n>> b/include/libcamera/internal/global_configuration.h\n>> index 89984c84d..357119628 100644\n>> --- a/include/libcamera/internal/global_configuration.h\n>> +++ b/include/libcamera/internal/global_configuration.h\n>> @@ -8,6 +8,8 @@\n>> #pragma once\n>> #include \n>> +#include \n>> +#include \n>> #include \"libcamera/internal/yaml_parser.h\"\n>> @@ -22,6 +24,9 @@ public:\n>> \tunsigned int version() const;\n>> \tConfiguration configuration() const;\n>> +\tstd::optional option(const std::string &confPath) const;\n>> +\tstd::optional envOption(const char *const envVariable,\n>> +\t\t\t\t\t const std::string &confPath) const;\n>> private:\n>> \tbool loadFile(const std::filesystem::path &fileName);\n>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp\n>> index 72b4a5246..068ddd5a5 100644\n>> --- a/src/libcamera/global_configuration.cpp\n>> +++ b/src/libcamera/global_configuration.cpp\n>> @@ -8,6 +8,8 @@\n>> #include \"libcamera/internal/global_configuration.h\"\n>> #include \n>> +#include \n>> +#include \n>> #include \n>> #include \n>> @@ -39,6 +41,11 @@ LOG_DEFINE_CATEGORY(Configuration)\n>> * is not found then in system-wide configuration directories. If multiple\n>> * configuration files exist then only the first one found is used and no\n>> * configuration merging is performed.\n>> + *\n>> + * The configuration can be accessed using the provided helpers. Namely\n>> + * GlobalConfiguration::option() or GlobalConfiguration::envOption() to access\n>> + * individual options or GlobalConfiguration::configuration() to access the\n>> + * whole configuration.\n>> */\n>> bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)\n>> @@ -108,6 +115,51 @@ GlobalConfiguration::GlobalConfiguration()\n>> * the underlying type.\n>> */\n>> +/**\n>> + * \\brief Return value of the configuration option identified by \\a confPath\n>> + * \\param[in] confPath Sequence of the YAML section names (excluding\n>> + * `configuration') leading to the requested option separated by slashes\n>> + * \\return A value if an item corresponding to \\a confPath exists in the\n>> + * configuration file, no value otherwise\n>> + */\n>> +std::optional GlobalConfiguration::option(const std::string &confPath) const\n>\n> I think we should avoid in-band signalling, so I would prefer removing the separators from the\n> string. And instead making the function take an `std::initializer_list` (and\n> `Span` if an init list not sufficient, but I think it should be).\n> I believe it is inevitable that someone will forget to consider if the input is \"safe\",\n> especially if not all path components are fixed.\n\nA good idea, will do.\n\n>> +{\n>> +\tconst YamlObject *c = &configuration();\n>> +\tfor (auto part : utils::split(confPath, \"/\")) {\n>> +\t\tc = &(*c)[part];\n>> +\t\tif (!*c)\n>> +\t\t\treturn {};\n>> +\t}\n>> +\treturn c->get();\n>> +}\n>> +\n>> +/**\n>> + * \\brief Return value of the configuration option from a file or environment\n>> + * \\param[in] envVariable Environment variable to get the value from\n>> + * \\param[in] confPath The same as in GlobalConfiguration::option\n>> + *\n>> + * This helper looks first at the given environment variable and if it is\n>> + * defined then it returns its value (even if it is empty). Otherwise it looks\n>> + * for \\a confPath the same way as in GlobalConfiguration::option. Only string\n>> + * values are supported.\n>> + *\n>> + * \\note Support for using environment variables to configure libcamera behavior\n>> + * is provided here mostly for backward compatibility reasons. Introducing new\n>> + * configuration environment variables is discouraged.\n>> + *\n>> + * \\return A value retrieved from the given environment option or configuration\n>> + * file or no value if not found\n>> + */\n>> +std::optional GlobalConfiguration::envOption(\n>> +\tconst char *const envVariable,\n>\n> Could you please drop the top-level `const`?\n\nOK.\n\n> Regards,\n> Barnabás Pőcze\n>\n>\n>> +\tconst std::string &confPath) const\n>> +{\n>> +\tconst char *envValue = utils::secure_getenv(envVariable);\n>> +\tif (envValue)\n>> +\t\treturn std::optional{ std::string{ envValue } };\n>> +\treturn option(confPath);\n>> +}\n>> +\n>> /**\n>> * \\brief Return configuration version\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 ABA2CC3237\n\tfor ;\n\tFri, 27 Jun 2025 18:57:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A458D68E04;\n\tFri, 27 Jun 2025 20:57:42 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B495868DE5\n\tfor ;\n\tFri, 27 Jun 2025 20:57:40 +0200 (CEST)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-372-19cnxFToP_ufwSfJGcCnNw-1; Fri, 27 Jun 2025 14:57:38 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-452ff9e054eso9922335e9.2\n\tfor ;\n\tFri, 27 Jun 2025 11:57:37 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-453883d81besm69335525e9.38.2025.06.27.11.57.35\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 27 Jun 2025 11:57:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"L0j1AnCH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1751050659;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=E3npG7ts3ZB9pHHi54PXtg/+l+E+IP2iX1U5BEf8GSE=;\n\tb=L0j1AnCHH0KNJ0dzBTbDeLPOtH1R/sLMDuFaJjwGebfrHF7OBwparN/wzMmQxFXgwf7FEP\n\teTw1Wht//V42oRGtXV6itwuXtOnis4+oDqitP0emvXvWGbFw5TAhkt39VV2Zph7NJU/tVK\n\t1kwdlWn6wV+8yry7B7/1ke3byR0Ml5Y=","X-MC-Unique":"19cnxFToP_ufwSfJGcCnNw-1","X-Mimecast-MFC-AGG-ID":"19cnxFToP_ufwSfJGcCnNw_1751050657","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1751050657; x=1751655457;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=wwL860fsEw61ZMl0/jpOM41ulmL3n+Ay54x4QOmzFIU=;\n\tb=l2XAu3wfkrIfbrp3ba6OehMdp1ePGFm0lW36GgvrsCqpYUDWFdt0NghAUV1/Dcb2ls\n\tYO0IPTCWuLkLOSKjSL0knMCb10Er2WsU5aWt5RNxOOnsTR3wVcJuAQr+H9pfyuB8ZP1F\n\tLLftzBDOsFPC9sQHxh/shfeL8yVQDUKAF9LsT3AWndC96rmqXYE4FRtFr+3OscadLkCs\n\tg/wy6IsvlR+Uq/7f94yLinD638iOVIU7zn4yTGL32mW3IomCbMZPoVlDUnhxsziNCa9T\n\t+SNlIVXTuCOndzsy1BoyIPOhvGiEhdZA25ZbojsVVPszCATc9CfdBj1lEDF7wTQieWsh\n\tqOLw==","X-Gm-Message-State":"AOJu0YyuYYKePVgSrVUKHUmn1BzqJWgoX6Rom5DZS8TRuMjQCGkq6DPW\n\t6m3f9JUP2qc5uCRmUzuLTcfcGsaQ7LpXJc3cJETw/swYbxU5pd7ZhUs8MijV2yKVF6xXG+adDg1\n\tAAd7CsZmpCEAs9FViphfAYg2/+a6VXV6BkFg8926wH+EEkt7pmetd2CmUSrsfVoXsaK4NPjq5UT\n\tQ=","X-Gm-Gg":"ASbGncv34Shseepl58CQRRG+x3sgLW0CIcBToJv7S0avrDyHB+XDLJ71grbibM5Melp\n\tV6AG+/hnZ7QWHQUPRw5+uyGpBDp7L64pomxijHT2vmNUsx6k2t7jjjNfkHyPp+WMPW3sB5531qc\n\tvqDHSNMJ56NsN8RwXMwxTIvOsI6ph9npM/uxBh8XO+EmWbwEWbwa91lQzGl1/0bggMVb1HPEQTT\n\tmcXMf65A3kgxgPAoUbSTUYM9AJhuTLGFSgTAwMP/3h7DDDF9gclpYChzdys0QA6PE0+NsWt3lPy\n\tq2uF8yS+uaUeL/ECOCio7Jvyk7UzuG+uxUGBdaZOQuIU9G1WA6esoVtvjj/gPDbsoMjjZbfcAww\n\t=","X-Received":["by 2002:a05:600c:3482:b0:43b:c0fa:f9dd with SMTP id\n\t5b1f17b1804b1-4538ee9663fmr41453005e9.25.1751050656719; \n\tFri, 27 Jun 2025 11:57:36 -0700 (PDT)","by 2002:a05:600c:3482:b0:43b:c0fa:f9dd with SMTP id\n\t5b1f17b1804b1-4538ee9663fmr41452835e9.25.1751050656267; \n\tFri, 27 Jun 2025 11:57:36 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEYVbIWPwUEWrr+QZpQcwNe6VPsrZbz5W1TcLGMxd/rXcX3oEre/wrgvIr0HdETblp3odsIdQ==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v11 03/12] config: Add configuration retrieval helpers","In-Reply-To":" (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Fri,\n\t27 Jun 2025 16:00:44 +0200\")","References":"<20250624083612.27230-1-mzamazal@redhat.com>\n\t<20250624083612.27230-4-mzamazal@redhat.com>\n\t","Date":"Fri, 27 Jun 2025 20:57:35 +0200","Message-ID":"<85frflvwps.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"_qAUm9kj1z5mA_ts_kvpZQNzhu8Dur31iesuI1p6BSs_1751050657","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]