Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virlog.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index ac35e36148c6..90d3d7c5cb53 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1187,6 +1187,10 @@ virLogParseDefaultPriority(const char *priority) return VIR_LOG_WARN; else if (STREQ(priority, "4") || STREQ(priority, "error")) return VIR_LOG_ERROR; + + virReportError(VIR_ERR_INVALID_ARG, + _("Failed to set logging priority, argument '%s' is " + "invalid"), priority); return -1; } -- 2.34.1

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virlog.c | 8 +++++--- src/util/virlog.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 90d3d7c5cb53..bf791d901a24 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -201,7 +201,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool privileged) * according to @binary, @godaemon, @privileged. This function should be run * exactly once at daemon startup, so no locks are used. */ -void +int virLogSetDefaultOutput(const char *binary, bool godaemon, bool privileged) { bool have_journald = access("/run/systemd/journal/socket", W_OK) >= 0; @@ -209,14 +209,16 @@ virLogSetDefaultOutput(const char *binary, bool godaemon, bool privileged) if (godaemon) { if (have_journald) virLogSetDefaultOutputToJournald(); - else - virLogSetDefaultOutputToFile(binary, privileged); + else if (virLogSetDefaultOutputToFile(binary, privileged) < 0) + return -1; } else { if (!isatty(STDIN_FILENO) && have_journald) virLogSetDefaultOutputToJournald(); else virLogSetDefaultOutputToStderr(); } + + return 0; } diff --git a/src/util/virlog.h b/src/util/virlog.h index 460e54ba0501..a04811e4083c 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -154,7 +154,7 @@ void virLogFilterListFree(virLogFilter **list, int count); int virLogSetOutputs(const char *outputs); int virLogSetFilters(const char *filters); char *virLogGetDefaultOutput(void); -void virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged); +int virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged); /* * Internal logging API -- 2.34.1

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virlog.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index bf791d901a24..e368cada6024 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -178,6 +178,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool privileged) old_umask = umask(077); if (g_mkdir_with_parents(logdir, 0777) < 0) { umask(old_umask); + virReportSystemError(errno, "%s", _("Could not create log directory")); return -1; } umask(old_umask); -- 2.34.1

The only difference is that we are not going to be guaranteed that the mutex is normal (as opposed to recursive, although there is no system known to me that would default to recursive mutexes), but that was done only to find occasional errors (during runtime, back in 2010, commit 336fd879c00b). Functions using this mutex are mostly stable and unchanging, and it makes the virLogOnceInit() function only return 0 (or possibly abort in glib calls). On top of that we can assume that the virLogMutex is always initialized which enables us to be more consistent in some early error reporting. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virlog.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index e368cada6024..5848940c6ccb 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -125,7 +125,7 @@ static void virLogOutputToFd(virLogSource *src, /* * Logs accesses must be serialized though a mutex */ -virMutex virLogMutex; +static virMutex virLogMutex = VIR_MUTEX_INITIALIZER; void virLogLock(void) @@ -250,9 +250,6 @@ virLogPriorityString(virLogPriority lvl) static int virLogOnceInit(void) { - if (virMutexInit(&virLogMutex) < 0) - return -1; - virLogLock(); virLogDefaultPriority = VIR_LOG_DEFAULT; -- 2.34.1

This prevents starting any daemons with improper logging settings. This is desirable on its own, but will be even more beneficial when more functions start reporting errors and failing on them, coming up in following patches Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/locking/lock_daemon.c | 15 ++++++++------- src/logging/log_daemon.c | 15 ++++++++------- src/remote/remote_daemon.c | 15 ++++++++------- src/util/virdaemon.c | 31 ++++++++++++++++++++----------- src/util/virdaemon.h | 14 +++++++------- 5 files changed, 51 insertions(+), 39 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 107fb22bc2c9..ea81940a4325 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -913,13 +913,14 @@ int main(int argc, char **argv) { } VIR_FREE(remote_config_file); - virDaemonSetupLogging("virtlockd", - config->log_level, - config->log_filters, - config->log_outputs, - privileged, - verbose, - godaemon); + if (virDaemonSetupLogging("virtlockd", + config->log_level, + config->log_filters, + config->log_outputs, + privileged, + verbose, + godaemon) < 0) + exit(EXIT_FAILURE); if (!pid_file && virPidFileConstructPath(privileged, diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index de6bf082e89b..fe7fa8534aec 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -719,13 +719,14 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - virDaemonSetupLogging("virtlogd", - config->log_level, - config->log_filters, - config->log_outputs, - privileged, - verbose, - godaemon); + if (virDaemonSetupLogging("virtlogd", + config->log_level, + config->log_filters, + config->log_outputs, + privileged, + verbose, + godaemon) < 0) + exit(EXIT_FAILURE); if (!pid_file && virPidFileConstructPath(privileged, diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 4e10f3ad2307..8a4610da83c8 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -936,13 +936,14 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - virDaemonSetupLogging(DAEMON_NAME, - config->log_level, - config->log_filters, - config->log_outputs, - privileged, - verbose, - godaemon); + if (virDaemonSetupLogging(DAEMON_NAME, + config->log_level, + config->log_filters, + config->log_outputs, + privileged, + verbose, + godaemon) < 0) + exit(EXIT_FAILURE); /* Let's try to initialize global variable that holds the host's boot time. */ if (virHostBootTimeInit() < 0) { diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c index bb2df2eb2cab..795206301227 100644 --- a/src/util/virdaemon.c +++ b/src/util/virdaemon.c @@ -151,7 +151,7 @@ virDaemonForkIntoBackground(const char *argv0) * but if verbose or error debugging is asked for then also output * informational and debug messages. Default size if 64 kB. */ -void +int virDaemonSetupLogging(const char *daemon_name, unsigned int log_level, char *log_filters, @@ -160,7 +160,8 @@ virDaemonSetupLogging(const char *daemon_name, bool verbose, bool godaemon) { - virLogReset(); + if (virLogReset() < 0) + return -1; /* * Libvirtd's order of precedence is: @@ -169,15 +170,17 @@ virDaemonSetupLogging(const char *daemon_name, * Given the precedence, we must process the variables in the opposite * order, each one overriding the previous. */ - if (log_level != 0) - virLogSetDefaultPriority(log_level); + if (log_level != 0 && + virLogSetDefaultPriority(log_level) < 0) + return -1; /* In case the config is empty, both filters and outputs will become empty, * however we can't start with empty outputs, thus we'll need to define and * setup a default one. */ - ignore_value(virLogSetFilters(log_filters)); - ignore_value(virLogSetOutputs(log_outputs)); + if (virLogSetFilters(log_filters) < 0 || + virLogSetOutputs(log_outputs) < 0) + return -1; /* If there are some environment variables defined, use those instead */ virLogSetFromEnv(); @@ -185,16 +188,22 @@ virDaemonSetupLogging(const char *daemon_name, /* * Command line override for --verbose */ - if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) - virLogSetDefaultPriority(VIR_LOG_INFO); + if (verbose && + virLogGetDefaultPriority() > VIR_LOG_INFO && + virLogSetDefaultPriority(VIR_LOG_INFO) < 0) + return -1; /* Define the default output. This is only applied if there was no setting * from either the config or the environment. */ - virLogSetDefaultOutput(daemon_name, godaemon, privileged); + if (virLogSetDefaultOutput(daemon_name, godaemon, privileged) < 0) + return -1; + + if (virLogGetNbOutputs() == 0 && + virLogSetOutputs(virLogGetDefaultOutput()) < 0) + return -1; - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(virLogGetDefaultOutput()); + return 0; } diff --git a/src/util/virdaemon.h b/src/util/virdaemon.h index d032b8ddb3b9..9ed0942d6d0e 100644 --- a/src/util/virdaemon.h +++ b/src/util/virdaemon.h @@ -58,13 +58,13 @@ VIR_ENUM_IMPL(virDaemonErr, int virDaemonForkIntoBackground(const char *argv0); -void virDaemonSetupLogging(const char *daemon_name, - unsigned int log_level, - char *log_filters, - char *log_outputs, - bool privileged, - bool verbose, - bool godaemon); +int virDaemonSetupLogging(const char *daemon_name, + unsigned int log_level, + char *log_filters, + char *log_outputs, + bool privileged, + bool verbose, + bool godaemon); int virDaemonUnixSocketPaths(const char *sock_prefix, bool privileged, -- 2.34.1

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. Before this commit it is possible to deadlock the daemon on startup with something like: LIBVIRT_LOG_FILTERS='1:*' LIBVIRT_LOG_OUTPUTS=1:stdout libvirtd where filters are used to enable more logging and hence make the race less rare and outputs are set to invalid Combined with the previous patches this changes the following from: ... <deadlock> to: ... libvirtd: initialisation failed The error message is improved in future commits and is also possible thanks to this patch. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/admin/libvirt-admin.c | 3 ++- src/libvirt.c | 3 ++- src/lxc/lxc_controller.c | 3 ++- src/remote/remote_ssh_helper.c | 3 ++- src/security/virt-aa-helper.c | 3 ++- src/util/vircommand.c | 3 ++- src/util/virdaemon.c | 3 ++- src/util/virlog.c | 26 ++++++++++++++++++-------- src/util/virlog.h | 2 +- tests/testutils.c | 4 +++- 10 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c index a3164a2395cf..01546a7bc270 100644 --- a/src/admin/libvirt-admin.c +++ b/src/admin/libvirt-admin.c @@ -55,7 +55,8 @@ virAdmGlobalInit(void) if (virErrorInitialize() < 0) goto error; - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + goto error; #ifdef WITH_LIBINTL_H if (!bindtextdomain(PACKAGE, LOCALEDIR)) diff --git a/src/libvirt.c b/src/libvirt.c index ef9fc403d083..35fd74fe08da 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -232,7 +232,8 @@ virGlobalInit(void) goto error; } - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + goto error; virNetTLSInit(); diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 67ebed361ae0..3c930eaacdee 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2503,7 +2503,8 @@ int main(int argc, char *argv[]) } /* Initialize logging */ - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + exit(EXIT_FAILURE); while (1) { int c; diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c index 4f4dbff7d0ad..78f02020a745 100644 --- a/src/remote/remote_ssh_helper.c +++ b/src/remote/remote_ssh_helper.c @@ -402,7 +402,8 @@ int main(int argc, char **argv) virFileActivateDirOverrideForProg(argv[0]); /* Initialize the log system */ - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + exit(EXIT_FAILURE); uri_str = argv[1]; VIR_DEBUG("Using URI %s", uri_str); diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index b7ffb5e2c324..28717b7e38b8 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1449,7 +1449,8 @@ main(int argc, char **argv) virFileActivateDirOverrideForProg(argv[0]); /* Initialize the log system */ - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + exit(EXIT_FAILURE); /* clear the environment */ environ = NULL; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index ba5a2090768b..3b8c1c65c160 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -751,7 +751,8 @@ virExec(virCommand *cmd) VIR_FORCE_CLOSE(null); /* Initialize full logging for a while */ - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + goto fork_error; if (cmd->pidfile && virPipe(pipesync) < 0) diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c index 795206301227..5c0ca534cf16 100644 --- a/src/util/virdaemon.c +++ b/src/util/virdaemon.c @@ -183,7 +183,8 @@ virDaemonSetupLogging(const char *daemon_name, return -1; /* If there are some environment variables defined, use those instead */ - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + return -1; /* * Command line override for --verbose diff --git a/src/util/virlog.c b/src/util/virlog.c index 5848940c6ccb..71913a2997f4 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1200,24 +1200,34 @@ virLogParseDefaultPriority(const char *priority) * * Sets virLogDefaultPriority, virLogFilters and virLogOutputs based on * environment variables. + * + * This function might fail which should also make the caller fail. */ -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; + } 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; } diff --git a/src/util/virlog.h b/src/util/virlog.h index a04811e4083c..4f755c543b78 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -146,7 +146,7 @@ char *virLogGetFilters(void); char *virLogGetOutputs(void); virLogPriority virLogGetDefaultPriority(void); int virLogSetDefaultPriority(virLogPriority priority); -void virLogSetFromEnv(void); +int virLogSetFromEnv(void) G_GNUC_WARN_UNUSED_RESULT; void virLogOutputFree(virLogOutput *output); void virLogOutputListFree(virLogOutput **list, int count); void virLogFilterFree(virLogFilter *filter); diff --git a/tests/testutils.c b/tests/testutils.c index 65f02e023199..322c3b94004c 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -835,7 +835,9 @@ int virTestMain(int argc, if (virErrorInitialize() < 0) return EXIT_FAILURE; - virLogSetFromEnv(); + if (virLogSetFromEnv() < 0) + return EXIT_FAILURE; + if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) { if (!(output = virLogOutputNew(virtTestLogOutput, virtTestLogClose, &testLog, VIR_LOG_DEBUG, -- 2.34.1