Re: [PATCH 6/8] util: Check for errors in virLogSetFromEnv

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(a)redhat.com&gt;