[PATCH] Share Dup Daemon Function *SetupLogging

Create a new function virDaemonSetupLogging (src/util/virdaemon.c) for shared code in virLockDaemonSetupLogging (src/locking/lock_daemon.c) virLogDaemonSetupLogging (src/logging/log_daemon.c) daemonSetupLogging (src/remote/remote_daemon.c) One of the BiteSizedTasks --- src/libvirt_private.syms | 2 + src/locking/lock_daemon.c | 58 ++--------------------------- src/logging/log_daemon.c | 50 ++----------------------- src/remote/remote_daemon.c | 57 ++--------------------------- src/util/Makefile.inc.am | 4 +- src/util/virdaemon.c | 75 ++++++++++++++++++++++++++++++++++++++ src/util/virdaemon.h | 35 ++++++++++++++++++ 7 files changed, 126 insertions(+), 155 deletions(-) create mode 100644 src/util/virdaemon.c create mode 100644 src/util/virdaemon.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index de0c7a3133..50cbd6d7af 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1906,6 +1906,8 @@ virCryptoHashBuf; virCryptoHashString; virCryptoHaveCipher; +# util/virdaemon.h +virDaemonSetupLogging; # util/virdbus.h virDBusCallMethod; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 5e5a0c1089..5ba851cb55 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -46,6 +46,7 @@ #include "virstring.h" #include "virgettext.h" #include "virenum.h" +#include "virdaemon.h" #include "locking/lock_daemon_dispatch.h" #include "locking/lock_protocol.h" @@ -477,59 +478,6 @@ virLockDaemonErrorHandler(void *opaque G_GNUC_UNUSED, } -/* - * Set up the logging environment - * By default if daemonized all errors go to the logfile libvirtd.log, - * but if verbose or error debugging is asked for then also output - * informational and debug messages. Default size if 64 kB. - */ -static int -virLockDaemonSetupLogging(virLockDaemonConfigPtr config, - bool privileged, - bool verbose, - bool godaemon) -{ - virLogReset(); - - /* - * Libvirtd's order of precedence is: - * cmdline > environment > config - * - * Given the precedence, we must process the variables in the opposite - * order, each one overriding the previous. - */ - if (config->log_level != 0) - virLogSetDefaultPriority(config->log_level); - - /* 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(config->log_filters)); - ignore_value(virLogSetOutputs(config->log_outputs)); - - /* If there are some environment variables defined, use those instead */ - virLogSetFromEnv(); - - /* - * Command line override for --verbose - */ - if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) - virLogSetDefaultPriority(VIR_LOG_INFO); - - /* Define the default output. This is only applied if there was no setting - * from either the config or the environment. - */ - virLogSetDefaultOutput("virtlockd", godaemon, privileged); - - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(virLogGetDefaultOutput()); - - return 0; -} - - - /* Display version information. */ static void virLockDaemonVersion(const char *argv0) @@ -1186,7 +1134,9 @@ int main(int argc, char **argv) { } VIR_FREE(remote_config_file); - if (virLockDaemonSetupLogging(config, privileged, verbose, godaemon) < 0) { + if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)), + "virtlockd", privileged, + verbose, godaemon)< 0) { VIR_ERROR(_("Can't initialize logging")); exit(EXIT_FAILURE); } diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 772bbb805b..9f962300ed 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -45,6 +45,7 @@ #include "virstring.h" #include "virgettext.h" #include "virenum.h" +#include "virdaemon.h" #include "log_daemon_dispatch.h" #include "log_protocol.h" @@ -419,51 +420,6 @@ virLogDaemonErrorHandler(void *opaque G_GNUC_UNUSED, } -static void -virLogDaemonSetupLogging(virLogDaemonConfigPtr config, - bool privileged, - bool verbose, - bool godaemon) -{ - virLogReset(); - - /* - * Libvirtd's order of precedence is: - * cmdline > environment > config - * - * Given the precedence, we must process the variables in the opposite - * order, each one overriding the previous. - */ - if (config->log_level != 0) - virLogSetDefaultPriority(config->log_level); - - /* 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(config->log_filters)); - ignore_value(virLogSetOutputs(config->log_outputs)); - - /* If there are some environment variables defined, use those instead */ - virLogSetFromEnv(); - - /* - * Command line override for --verbose - */ - if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) - virLogSetDefaultPriority(VIR_LOG_INFO); - - /* Define the default output. This is only applied if there was no setting - * from either the config or the environment. - */ - virLogSetDefaultOutput("virtlogd", godaemon, privileged); - - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(virLogGetDefaultOutput()); -} - - - /* Display version information. */ static void virLogDaemonVersion(const char *argv0) @@ -957,8 +913,8 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - virLogDaemonSetupLogging(config, privileged, verbose, godaemon); - + virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)), + "virtlogd", privileged, verbose, godaemon); if (!pid_file && virPidFileConstructPath(privileged, RUNSTATEDIR, diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 7082460bae..4bbdc255d0 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -57,6 +57,7 @@ #include "util/virnetdevopenvswitch.h" #include "virsystemd.h" #include "virhostuptime.h" +#include "virdaemon.h" #include "driver.h" @@ -606,58 +607,6 @@ daemonSetupNetDevOpenvswitch(struct daemonConfig *config) } -/* - * Set up the logging environment - * By default if daemonized all errors go to journald/a logfile - * but if verbose or error debugging is asked for then also output - * informational and debug messages. Default size if 64 kB. - */ -static int -daemonSetupLogging(struct daemonConfig *config, - bool privileged, - bool verbose, - bool godaemon) -{ - virLogReset(); - - /* - * Logging setup order of precedence is: - * cmdline > environment > config - * - * Given the precedence, we must process the variables in the opposite - * order, each one overriding the previous. - */ - if (config->log_level != 0) - virLogSetDefaultPriority(config->log_level); - - /* 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(config->log_filters)); - ignore_value(virLogSetOutputs(config->log_outputs)); - - /* If there are some environment variables defined, use those instead */ - virLogSetFromEnv(); - - /* - * Command line override for --verbose - */ - if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) - virLogSetDefaultPriority(VIR_LOG_INFO); - - /* 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 (virLogGetNbOutputs() == 0) - virLogSetOutputs(virLogGetDefaultOutput()); - - return 0; -} - - static int daemonSetupAccessManager(struct daemonConfig *config) { @@ -1147,7 +1096,9 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - if (daemonSetupLogging(config, privileged, verbose, godaemon) < 0) { + if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)), + DAEMON_NAME, privileged, + verbose, godaemon) < 0) { VIR_ERROR(_("Can't initialize logging")); exit(EXIT_FAILURE); } diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index ddb3b43c5f..cf64220b63 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -42,7 +42,9 @@ UTIL_SOURCES = \ util/virconf.h \ util/vircrypto.c \ util/vircrypto.h \ - util/virdbus.c \ + util/virdaemon.c \ + util/virdaemon.h \ + util/virdbus.c \ util/virdbus.h \ util/virdbuspriv.h \ util/virdevmapper.c \ diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c new file mode 100644 index 0000000000..a2ef48b78b --- /dev/null +++ b/src/util/virdaemon.c @@ -0,0 +1,75 @@ +/* + * virdaemon.c: remote/logging/lock management daemon common code + * + * Copyright (C) 2006-2020 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "virdaemon.h" +#include "virlog.h" + + +/* + * Set up the logging environment + * By default if daemonized all errors go to journald/a logfile + * but if verbose or error debugging is asked for then also output + * informational and debug messages. Default size if 64 kB. + */ +int +virDaemonSetupLogging(virDaemonLogConfigPtr config, + const char *output_fname, + bool privileged, bool verbose, bool godaemon) +{ + virLogReset(); + + /* + * Libvirtd's order of precedence is: + * cmdline > environment > config + * + * Given the precedence, we must process the variables in the opposite + * order, each one overriding the previous. + */ + if (config->log_level != 0) + virLogSetDefaultPriority(config->log_level); + + /* 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(config->log_filters)); + ignore_value(virLogSetOutputs(config->log_outputs)); + + /* If there are some environment variables defined, use those instead */ + virLogSetFromEnv(); + + /* + * Command line override for --verbose + */ + if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) + virLogSetDefaultPriority(VIR_LOG_INFO); + + /* Define the default output. This is only applied if there was no setting + * from either the config or the environment. + */ + virLogSetDefaultOutput(output_fname, godaemon, privileged); + + if (virLogGetNbOutputs() == 0) + virLogSetOutputs(virLogGetDefaultOutput()); + + return 0; +} diff --git a/src/util/virdaemon.h b/src/util/virdaemon.h new file mode 100644 index 0000000000..60b4119f08 --- /dev/null +++ b/src/util/virdaemon.h @@ -0,0 +1,35 @@ +/* + * virdaemon.h: remote/logging/lock management daemon common code + * + * Copyright (C) 2006-2020 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once +#include "internal.h" + +typedef struct _virDaemonLogConfig virDaemonLogConfig; +typedef virDaemonLogConfig *virDaemonLogConfigPtr; + +struct _virDaemonLogConfig { + unsigned int log_level; + char *log_filters; + char *log_outputs; +}; + +int virDaemonSetupLogging(virDaemonLogConfigPtr config, + const char *output_fname, + bool privileged, bool verbose, bool godaemon); -- 2.17.1

On a Wednesday in 2020, Lan wrote:
Create a new function virDaemonSetupLogging (src/util/virdaemon.c) for shared code in virLockDaemonSetupLogging (src/locking/lock_daemon.c) virLogDaemonSetupLogging (src/logging/log_daemon.c) daemonSetupLogging (src/remote/remote_daemon.c)
As mentioned in our contributor guidelines (number 6. in general tips): https://libvirt.org/hacking.html we require a sign-off line with your legal name and e-mail address that tells us that we can legally use the patch you send us: https://developercertificate.org/
One of the BiteSizedTasks --- src/libvirt_private.syms | 2 + src/locking/lock_daemon.c | 58 ++--------------------------- src/logging/log_daemon.c | 50 ++----------------------- src/remote/remote_daemon.c | 57 ++--------------------------- src/util/Makefile.inc.am | 4 +- src/util/virdaemon.c | 75 ++++++++++++++++++++++++++++++++++++++ src/util/virdaemon.h | 35 ++++++++++++++++++ 7 files changed, 126 insertions(+), 155 deletions(-) create mode 100644 src/util/virdaemon.c create mode 100644 src/util/virdaemon.h
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index de0c7a3133..50cbd6d7af 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1906,6 +1906,8 @@ virCryptoHashBuf; virCryptoHashString; virCryptoHaveCipher;
+# util/virdaemon.h +virDaemonSetupLogging;
# util/virdbus.h virDBusCallMethod; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 5e5a0c1089..5ba851cb55 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -46,6 +46,7 @@ #include "virstring.h" #include "virgettext.h" #include "virenum.h" +#include "virdaemon.h"
#include "locking/lock_daemon_dispatch.h" #include "locking/lock_protocol.h" @@ -477,59 +478,6 @@ virLockDaemonErrorHandler(void *opaque G_GNUC_UNUSED, }
-/* - * Set up the logging environment - * By default if daemonized all errors go to the logfile libvirtd.log, - * but if verbose or error debugging is asked for then also output - * informational and debug messages. Default size if 64 kB. - */ -static int -virLockDaemonSetupLogging(virLockDaemonConfigPtr config, - bool privileged, - bool verbose, - bool godaemon) -{ - virLogReset(); - - /* - * Libvirtd's order of precedence is: - * cmdline > environment > config - * - * Given the precedence, we must process the variables in the opposite - * order, each one overriding the previous. - */ - if (config->log_level != 0) - virLogSetDefaultPriority(config->log_level); - - /* 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(config->log_filters)); - ignore_value(virLogSetOutputs(config->log_outputs)); - - /* If there are some environment variables defined, use those instead */ - virLogSetFromEnv(); - - /* - * Command line override for --verbose - */ - if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) - virLogSetDefaultPriority(VIR_LOG_INFO); - - /* Define the default output. This is only applied if there was no setting - * from either the config or the environment. - */ - virLogSetDefaultOutput("virtlockd", godaemon, privileged); - - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(virLogGetDefaultOutput()); - - return 0; -} - - - /* Display version information. */ static void virLockDaemonVersion(const char *argv0) @@ -1186,7 +1134,9 @@ int main(int argc, char **argv) { } VIR_FREE(remote_config_file);
- if (virLockDaemonSetupLogging(config, privileged, verbose, godaemon) < 0) { + if (virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)), + "virtlockd", privileged, + verbose, godaemon)< 0) {
This does not compile for me: ../../src/logging/log_daemon.c:916:27: error: cast from 'unsigned int *' to 'virDaemonLogConfigPtr' (aka 'struct _virDaemonLogConfig *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] virDaemonSetupLogging((virDaemonLogConfigPtr)(&(config->log_level)), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ config->log_level is an unsigned int virDaemonLogConfig is a structure While this might possibly work if the structs and stars are properly aligned, the clean, readable, and maintainable solution is to change all of the individual DaemonConfig structures to store the log_* variables in a virDaemonLogConfig structure. And only then change all the daemons to use the new virDaemonSetupLogging function. So this change will require a few separate commits: * Introduce virDaemonLogConfig (and the virdaemon files) * Use virDaemonLogConfig instead of the separate config->log_* variables * Pass virDaemonLogConfig instead of the daemon-specific config to all the SetupLogging functions * Introduce the new DaemonSetupLogging function and convert all the callers that now pass virDaemonLogConfig to use the new function. This approach will result in introducing lines that are later deleted by patches in the same series, but it makes the individual commits easier to read by the reviewer(s), and all the people that will need to look at the git history.
VIR_ERROR(_("Can't initialize logging")); exit(EXIT_FAILURE); } diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index ddb3b43c5f..cf64220b63 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -42,7 +42,9 @@ UTIL_SOURCES = \ util/virconf.h \ util/vircrypto.c \ util/vircrypto.h \ - util/virdbus.c \
This whitespace change looks suspicious. You should not need to touch the virdbus.c line at all. Jano
+ util/virdaemon.c \ + util/virdaemon.h \ + util/virdbus.c \ util/virdbus.h \ util/virdbuspriv.h \ util/virdevmapper.c \
participants (2)
-
Ján Tomko
-
Lan