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

[Barnabás Pőcze]

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.



Regards,
Barnabás Pőcze