[PATCH] Share Dup Daemon Function *SetupLogging

[Ján Tomko]

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 \
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200304/c0b263a1/attachment-0001.sig>