[PATCH v5 04/15] config: Look up logging levels in the configuration file

[Milan Zamazal]

Hi Barnabás,

Barnabás Pőcze <pobrn at protonmail.com> writes:

> Hi
>
>
> 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>
>> In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the
>> same configuration in the configuration file.
>> 
>> Because logging asks for configuration and configuration loading uses
>> logging, extra care is needed.  We must prevent calling any log function
>> before configuration is loaded.  Otherwise the log function would ask
>> for configuration, it would load the configuration files and would try
>> to log the results.  Which can lead to various breakages, even quite
>> subtle; this has been demonstrated in camera_reconfigure test when
>> trying to make the configuration initialization more transparent, as a
>> static variable instance with automatic initialization rather than a
>> pointer with manual initialization() invocation.
>> 
>> But we must log at least errors when reading the configuration.
>> Especially, we cannot prevent logging in configuration initialization
>> entirely because the YAML parser may log errors.  But in case of an
>> error during configuration initialization, the configuration cannot be
>> read anyway and the statically created initial global configuration
>> instance serves the purpose of providing some, empty configuration.
>> Then the errors from the YAML parser can be logged.
>> 
>> We cannot use the same mechanism for other logging in configuration
>> initialization because then logging would read and use the empty
>> configuration, which is not a valid configuration this time, for the
>> whole run of libcamera.
>> 
>> The configuration must be initialized, which is done by calling
>> initialize() method in IPAManager constructor.  A more suitable place
>> would be CameraManager constructor but it would be too late there, the
>> IPAManager constructor is called earlier.
>> 
>> The configuration snippet for logging looks as follows:
>> 
>>   configuration:
>>     log:
>>       levels: LIBCAMERA_LOG_LEVELS
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>> [...]
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index cfc24d389..33ae74e8f 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -16,6 +16,7 @@
>>  #include <libcamera/base/log.h>
>>  #include <libcamera/base/utils.h>
>> 
>> +#include "libcamera/internal/global_configuration.h"
>>  #include "libcamera/internal/ipa_module.h"
>>  #include "libcamera/internal/ipa_proxy.h"
>>  #include "libcamera/internal/pipeline_handler.h"
>> @@ -103,6 +104,8 @@ LOG_DEFINE_CATEGORY(IPAManager)
>>   */
>>  IPAManager::IPAManager()
>>  {
>> +	GlobalConfiguration::initialize();
>> [...]
>
> This looks very fragile. I think there is an appreciable risk of an unrelated change
> upsetting the order of construction so that this no longer works as expected.

Yes, but does anybody have any idea how to do better?

> Regards,
> Barnabás Pőcze