On Tue, Jan 04, 2022 at 02:47:10PM +0100, Martin Kletzander wrote:
> And make callers check the return value as well. This helps error out early for
> invalid environment variables.
>
> That is desirable because it could lead to deadlocks. This can happen when
> resetting logging after fork() reports translated errors because gettext
> functions are not reentrant. Well, it is not limited to resetting logging after
> fork(), it can be any translation at that phase, but parsing environment
> variables is easy to make fail on purpose to show the result, it can also happen
> just due to a typo.
> Logging settings are also something that we want to report
> errors on for example when it is being done over admin API.
True in general, but slightly off-topic wrt to the patch itself as
virLogSetFromEnv is irrelevant to admin API usage.
...
> -void
> +int
> virLogSetFromEnv(void)
> {
> const char *debugEnv;
>
> if (virLogInitialize() < 0)
> - return;
> + return -1;
>
> debugEnv = getenv("LIBVIRT_DEBUG");
> - if (debugEnv && *debugEnv)
> - virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv));
> + if (debugEnv && *debugEnv) {
> + int priority = virLogParseDefaultPriority(debugEnv);
> + if (priority < 0 ||
> + virLogSetDefaultPriority(priority) < 0)
> + return -1;
^^^ indentation
> + }
> debugEnv = getenv("LIBVIRT_LOG_FILTERS");
> - if (debugEnv && *debugEnv)
> - virLogSetFilters(debugEnv);
> + if (debugEnv && *debugEnv &&
> + virLogSetFilters(debugEnv))
> + return -1;
> debugEnv = getenv("LIBVIRT_LOG_OUTPUTS");
> - if (debugEnv && *debugEnv)
> - virLogSetOutputs(debugEnv);
> + if (debugEnv && *debugEnv &&
> + virLogSetOutputs(debugEnv))
> + return -1;
> +
> + return 0;
> }
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
