[{"id":32527,"web_url":"https://patchwork.libcamera.org/comment/32527/","msgid":"","date":"2024-12-04T17:45:02","subject":"Re: [PATCH v5 04/15] config: Look up logging levels in the\n\tconfiguration file","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal írta:\n\n> In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the\n> same configuration in the configuration file.\n> \n> Because logging asks for configuration and configuration loading uses\n> logging, extra care is needed. We must prevent calling any log function\n> before configuration is loaded. Otherwise the log function would ask\n> for configuration, it would load the configuration files and would try\n> to log the results. Which can lead to various breakages, even quite\n> subtle; this has been demonstrated in camera_reconfigure test when\n> trying to make the configuration initialization more transparent, as a\n> static variable instance with automatic initialization rather than a\n> pointer with manual initialization() invocation.\n> \n> But we must log at least errors when reading the configuration.\n> Especially, we cannot prevent logging in configuration initialization\n> entirely because the YAML parser may log errors. But in case of an\n> error during configuration initialization, the configuration cannot be\n> read anyway and the statically created initial global configuration\n> instance serves the purpose of providing some, empty configuration.\n> Then the errors from the YAML parser can be logged.\n> \n> We cannot use the same mechanism for other logging in configuration\n> initialization because then logging would read and use the empty\n> configuration, which is not a valid configuration this time, for the\n> whole run of libcamera.\n> \n> The configuration must be initialized, which is done by calling\n> initialize() method in IPAManager constructor. A more suitable place\n> would be CameraManager constructor but it would be too late there, the\n> IPAManager constructor is called earlier.\n> \n> The configuration snippet for logging looks as follows:\n> \n> configuration:\n> log:\n> levels: LIBCAMERA_LOG_LEVELS\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> [...]\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index cfc24d389..33ae74e8f 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/global_configuration.h\"\n> #include \"libcamera/internal/ipa_module.h\"\n> #include \"libcamera/internal/ipa_proxy.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -103,6 +104,8 @@ LOG_DEFINE_CATEGORY(IPAManager)\n> */\n> IPAManager::IPAManager()\n> {\n> +\tGlobalConfiguration::initialize();\n> [...]\n\nThis looks very fragile. I think there is an appreciable risk of an unrelated change\nupsetting the order of construction so that this no longer works as expected.\n\n\n\nRegards,\nBarnabás Pőcze","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 ABDD6C32BB\n\tfor ;\n\tWed, 4 Dec 2024 17:45:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50FE7660EB;\n\tWed, 4 Dec 2024 18:45:19 +0100 (CET)","from mail-10628.protonmail.ch (mail-10628.protonmail.ch\n\t[79.135.106.28])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21E54660DA\n\tfor ;\n\tWed, 4 Dec 2024 18:45:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"eqqY6hcN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1733334311; x=1733593511;\n\tbh=su9pjyXVd42fCU3Npbo7CjILldwK9rrdm49CgyglIB4=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=eqqY6hcNi4NRKynakUCaK+IBZkERPanNgX/hwLm6ChfrCb07VBQT/df2SJKVYdEAh\n\tg4cbrwGN8pMrbGP9ZZOeLAJzpTWxarrNHCDSr6AMCxO+9CT0Yvfq38uhKDx961EoRd\n\tq9zgSdEZu+RQxird9sSV98GVWE4iXfXVcrJuQd/OFUHj9wwKKzXs1mAf8WlvlXrUB3\n\twvMBaq42m3jdVpIyd+PpJnVJzLW6vgnasKoKLzH92yspTQ/iD89YaAa+gIOIBqj4Xi\n\tYhGCQMSgq7Mp/dh0OCtFX5A9JbRXrQfjGV7H8olgR+zlkZySiHqylI94qpmUd1PuLR\n\tw5gC4mkh85AyQ==","Date":"Wed, 04 Dec 2024 17:45:02 +0000","To":"Milan Zamazal ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tNaushir Patuck ","Subject":"Re: [PATCH v5 04/15] config: Look up logging levels in the\n\tconfiguration file","Message-ID":"","In-Reply-To":"<20241001102810.479285-5-mzamazal@redhat.com>","References":"<20241001102810.479285-1-mzamazal@redhat.com>\n\t<20241001102810.479285-5-mzamazal@redhat.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"ddc080aab301bc2706113a63277ebf43039615fd","MIME-Version":"1.0","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\" "}},{"id":32541,"web_url":"https://patchwork.libcamera.org/comment/32541/","msgid":"<87o71qz2ea.fsf@redhat.com>","date":"2024-12-05T13:26:53","subject":"Re: [PATCH v5 04/15] config: Look up logging levels in the\n\tconfiguration file","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Barnabás,\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n>\n> 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal írta:\n>\n>> In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the\n>> same configuration in the configuration file.\n>> \n>> Because logging asks for configuration and configuration loading uses\n>> logging, extra care is needed. We must prevent calling any log function\n>> before configuration is loaded. Otherwise the log function would ask\n>> for configuration, it would load the configuration files and would try\n>> to log the results. Which can lead to various breakages, even quite\n>> subtle; this has been demonstrated in camera_reconfigure test when\n>> trying to make the configuration initialization more transparent, as a\n>> static variable instance with automatic initialization rather than a\n>> pointer with manual initialization() invocation.\n>> \n>> But we must log at least errors when reading the configuration.\n>> Especially, we cannot prevent logging in configuration initialization\n>> entirely because the YAML parser may log errors. But in case of an\n>> error during configuration initialization, the configuration cannot be\n>> read anyway and the statically created initial global configuration\n>> instance serves the purpose of providing some, empty configuration.\n>> Then the errors from the YAML parser can be logged.\n>> \n>> We cannot use the same mechanism for other logging in configuration\n>> initialization because then logging would read and use the empty\n>> configuration, which is not a valid configuration this time, for the\n>> whole run of libcamera.\n>> \n>> The configuration must be initialized, which is done by calling\n>> initialize() method in IPAManager constructor. A more suitable place\n>> would be CameraManager constructor but it would be too late there, the\n>> IPAManager constructor is called earlier.\n>> \n>> The configuration snippet for logging looks as follows:\n>> \n>> configuration:\n>> log:\n>> levels: LIBCAMERA_LOG_LEVELS\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> [...]\n>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n>> index cfc24d389..33ae74e8f 100644\n>> --- a/src/libcamera/ipa_manager.cpp\n>> +++ b/src/libcamera/ipa_manager.cpp\n>> @@ -16,6 +16,7 @@\n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/global_configuration.h\"\n>> #include \"libcamera/internal/ipa_module.h\"\n>> #include \"libcamera/internal/ipa_proxy.h\"\n>> #include \"libcamera/internal/pipeline_handler.h\"\n>> @@ -103,6 +104,8 @@ LOG_DEFINE_CATEGORY(IPAManager)\n>> */\n>> IPAManager::IPAManager()\n>> {\n>> +\tGlobalConfiguration::initialize();\n>> [...]\n>\n> This looks very fragile. I think there is an appreciable risk of an unrelated change\n> upsetting the order of construction so that this no longer works as expected.\n\nYes, but does anybody have any idea how to do better?\n\n> Regards,\n> Barnabás Pőcze","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 3A814BDB1C\n\tfor ;\n\tThu, 5 Dec 2024 13:27:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CACB660FD;\n\tThu, 5 Dec 2024 14:27:02 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14CE16608C\n\tfor ;\n\tThu, 5 Dec 2024 14:26:59 +0100 (CET)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-153-_j9-K8PpMUyXfJZZ05RHrw-1; Thu, 05 Dec 2024 08:26:58 -0500","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5d0b6f3770eso513359a12.3\n\tfor ;\n\tThu, 05 Dec 2024 05:26:57 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5d149a49dc6sm780449a12.31.2024.12.05.05.26.54\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 05 Dec 2024 05:26:54 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"RQ06wrf0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1733405219;\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=Dbu1Joz37rD8rHMtS2C25pIi7NgCNmbyRajxkj9qxn4=;\n\tb=RQ06wrf0atJ1kPN2hjRDYobxM8ZMoftCeel5mzSDbMHZt83ygrL/o/Vr6GMPl53+arRJsH\n\tCj2Lj9rtDqE9d+xoslKqwavySC75nRR0IiOFXCKq/TVR8uHV2dDZ8Jaq9xe7tPWvqYr9tE\n\tBErPbY4OZ6bgAsS3cE3U5BuL5uov/uI=","X-MC-Unique":"_j9-K8PpMUyXfJZZ05RHrw-1","X-Mimecast-MFC-AGG-ID":"_j9-K8PpMUyXfJZZ05RHrw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733405216; x=1734010016;\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=5FszSc7fKa+sybNuQAzAdS90aRKoBR7TnGEVoNxvuvE=;\n\tb=HRVg/nfN8HW+LvX5gR8i/1FnF+k3EjZDBNZ8JMs5jwXy3F/ARSHYLqGlTfYfAVtNhk\n\tZp/97mxS7Cnu0+668Ve1uPZvRUtJIGNr5LEs9EaEBptL10sXtgH/s7ojxtwRwYT7Qdd+\n\ty9Afxv4iZozB2GkoJ9iOf3THs6xErIGT8Z/pLR0ccnO/YVGsmCun/xZpQS6/ZXI4Hz99\n\tLUfmAkOFLJOB9Plb40C4m70IAMMUkYD3XRgAXzaN47l4z+aZraSI8wwFEnxX3TTPFYTm\n\t29bBqAxASQJHjZpfnl9pruK04OQNPFyx+w72knbP/x5d4HTKpjq1lDaBxXN6SNJAhidC\n\tigSA==","X-Gm-Message-State":"AOJu0YxwpEicMH/G+RK8twrJtjyiBC5Rjn2LyWHXCOGt1Zs3IwsTOhzS\n\tYRCIgrFA7A9WSDiPavDoDA6ggWsAU4pQYjWrn5CTnDWNpOHoJQGShGW4Q6YkGwS7ZOX2OKnyujM\n\tbEEQqfdXIeYDHDEQr1ii1TH/fUY2OVgQfHZRQBZSVyGDLdlpRoQ9BLaECC/vfsK4scTP2CLjBth\n\t+ic1g=","X-Gm-Gg":"ASbGncuNAe+6cU+vWgb5ZI4diCuuxNk9taz90ODPK/5KS83TbeWk7OZ+XEjXxU/sMWi\n\t+IZna4VtlCE/3ep/HVw1v07pPooM12BNkQxSk3o9QtiNpCji6j8TocMOn9Lj3O9ceuQFEVaVy1x\n\tjhwC7ZB3aAkHKrArKU+EPAao4q7xAQ8KZiA8IFGC/CN1l3vPzh8qgzj/bgNKaI7e0En24W3STZ9\n\tTQZwjb4dL9f5j+rnDDeNnBP27Bl6BA1ruDRomCKqWbGo4G32SmqI0EDToIueu0MA4ZFCb0=","X-Received":["by 2002:a05:6402:4024:b0:5d2:729f:995f with SMTP id\n\t4fb4d7f45d1cf-5d2729f9b6bmr1265167a12.29.1733405216460; \n\tThu, 05 Dec 2024 05:26:56 -0800 (PST)","by 2002:a05:6402:4024:b0:5d2:729f:995f with SMTP id\n\t4fb4d7f45d1cf-5d2729f9b6bmr1265146a12.29.1733405216048; \n\tThu, 05 Dec 2024 05:26:56 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHpPON1LCGE3at9zn3jlGwMDZKcArxGrAH5/cvYmipdVA1o9t66Q+RV+efBg2nEPtQLu42ICQ==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Naushir Patuck ","Subject":"Re: [PATCH v5 04/15] config: Look up logging levels in the\n\tconfiguration file","In-Reply-To":"\n\t( =?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Wed,\n\t04 Dec 2024 17:45:02 +0000\")","References":"<20241001102810.479285-1-mzamazal@redhat.com>\n\t<20241001102810.479285-5-mzamazal@redhat.com>\n\t","Date":"Thu, 05 Dec 2024 14:26:53 +0100","Message-ID":"<87o71qz2ea.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"cFoI7Gv93hm9KpCAMGVhaUX-_Iy5Pp_xe0BhYSHJaCk_1733405217","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\" "}}]