[libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon

Daniel P. Berrangé posted 41 patches 6 years, 6 months ago
There is a newer version of this series
[libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
Posted by Daniel P. Berrangé 6 years, 6 months ago
Prepare for reusing libvirtd source to create other daemons by making
the daemon name conditionally defined by the make rules.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/remote/Makefile.inc.am        |  1 +
 src/remote/remote_daemon.c        | 48 +++++++++++++++++--------------
 src/remote/remote_daemon_config.c |  5 ++--
 src/remote/remote_driver.h        |  1 -
 4 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index ced940d3c1..b72186109a 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -147,6 +147,7 @@ libvirtd_CFLAGS = \
 	-I$(srcdir)/conf \
 	-I$(srcdir)/rpc \
 	-DSOCK_PREFIX="\"libvirt\"" \
+	-DDAEMON_NAME="\"libvirtd\"" \
 	$(NULL)
 
 libvirtd_LDFLAGS = \
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 056ae2ba91..0982e281de 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -64,7 +64,11 @@
 
 #include "virdbus.h"
 
-VIR_LOG_INIT("daemon.libvirtd");
+VIR_LOG_INIT("daemon." DAEMON_NAME);
+
+#ifndef SOCK_PREFIX
+# define SOCK_PREFIX DAEMON_NAME
+#endif
 
 #if WITH_SASL
 virNetSASLContextPtr saslCtxt = NULL;
@@ -375,11 +379,11 @@ daemonSetupNetworking(virNetServerPtr srv,
     int ret = -1;
     VIR_AUTOPTR(virSystemdActivation) act = NULL;
     virSystemdActivationMap actmap[] = {
-        { .name = "libvirtd.socket", .family = AF_UNIX, .path = sock_path },
-        { .name = "libvirtd-ro.socket", .family = AF_UNIX, .path = sock_path_ro },
-        { .name = "libvirtd-admin.socket", .family = AF_UNIX, .path = sock_path_adm },
-        { .name = "libvirtd-tcp.socket", .family = AF_INET },
-        { .name = "libvirtd-tls.socket", .family = AF_INET },
+        { .name = DAEMON_NAME ".socket", .family = AF_UNIX, .path = sock_path },
+        { .name = DAEMON_NAME "-ro.socket", .family = AF_UNIX, .path = sock_path_ro },
+        { .name = DAEMON_NAME "-admin.socket", .family = AF_UNIX, .path = sock_path_adm },
+        { .name = DAEMON_NAME "-tcp.socket", .family = AF_INET },
+        { .name = DAEMON_NAME "-tls.socket", .family = AF_INET },
     };
 
     if ((actmap[3].port = virSocketAddrResolveService(config->tcp_port)) < 0)
@@ -413,7 +417,7 @@ daemonSetupNetworking(virNetServerPtr srv,
 
     if (virNetServerAddServiceUNIX(srv,
                                    act,
-                                   "libvirtd.socket",
+                                   DAEMON_NAME ".socket",
                                    sock_path,
                                    unix_sock_rw_mask,
                                    unix_sock_gid,
@@ -426,7 +430,7 @@ daemonSetupNetworking(virNetServerPtr srv,
     if (sock_path_ro &&
         virNetServerAddServiceUNIX(srv,
                                    act,
-                                   "libvirtd-ro.socket",
+                                   DAEMON_NAME "-ro.socket",
                                    sock_path_ro,
                                    unix_sock_ro_mask,
                                    unix_sock_gid,
@@ -440,7 +444,7 @@ daemonSetupNetworking(virNetServerPtr srv,
     if (sock_path_adm &&
         virNetServerAddServiceUNIX(srvAdm,
                                    act,
-                                   "libvirtd-admin.socket",
+                                   DAEMON_NAME "-admin.socket",
                                    sock_path_adm,
                                    unix_sock_adm_mask,
                                    unix_sock_gid,
@@ -454,7 +458,7 @@ daemonSetupNetworking(virNetServerPtr srv,
     if (((ipsock && config->listen_tcp) || act) &&
         virNetServerAddServiceTCP(srv,
                                   act,
-                                  "libvirtd-tcp.socket",
+                                  DAEMON_NAME "-tcp.socket",
                                   config->listen_addr,
                                   config->tcp_port,
                                   AF_UNSPEC,
@@ -511,7 +515,7 @@ daemonSetupNetworking(virNetServerPtr srv,
                   config->listen_addr, config->tls_port);
         if (virNetServerAddServiceTCP(srv,
                                       act,
-                                      "libvirtd-tls.socket",
+                                      DAEMON_NAME "-tls.socket",
                                       config->listen_addr,
                                       config->tls_port,
                                       AF_UNSPEC,
@@ -556,7 +560,7 @@ daemonSetupNetDevOpenvswitch(struct daemonConfig *config)
 
 /*
  * Set up the logging environment
- * By default if daemonized all errors go to the logfile libvirtd.log,
+ * 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.
  */
@@ -569,7 +573,7 @@ daemonSetupLogging(struct daemonConfig *config,
     virLogReset();
 
     /*
-     * Libvirtd's order of precedence is:
+     * Logging setup order of precedence is:
      * cmdline > environment > config
      *
      * Given the precedence, we must process the variables in the opposite
@@ -597,7 +601,7 @@ daemonSetupLogging(struct daemonConfig *config,
     /* Define the default output. This is only applied if there was no setting
      * from either the config or the environment.
      */
-    if (virLogSetDefaultOutput("libvirtd", godaemon, privileged) < 0)
+    if (virLogSetDefaultOutput(DAEMON_NAME, godaemon, privileged) < 0)
         return -1;
 
     if (virLogGetNbOutputs() == 0)
@@ -709,7 +713,7 @@ static void daemonStopWorker(void *opaque)
 
     VIR_DEBUG("Completed stop dmn=%p", dmn);
 
-    /* Exit libvirtd cleanly */
+    /* Exit daemon cleanly */
     virNetDaemonQuit(dmn);
 }
 
@@ -788,7 +792,7 @@ static void daemonRunStateInit(void *opaque)
     driversInitialized = true;
 
 #ifdef WITH_DBUS
-    /* Tie the non-privileged libvirtd to the session/shutdown lifecycle */
+    /* Tie the non-privileged daemons to the session/shutdown lifecycle */
     if (!virNetDaemonIsPrivileged(dmn)) {
 
         sessionBus = virDBusGetSessionBus();
@@ -895,7 +899,7 @@ daemonUsage(const char *argv0, bool privileged)
     fprintf(stderr, "\n");
 
     fprintf(stderr, "    %s:\n", _("Configuration file (unless overridden by -f)"));
-    fprintf(stderr, "      %s/libvirt/libvirtd.conf\n",
+    fprintf(stderr, "      %s/libvirt/" DAEMON_NAME ".conf\n",
             privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");
     fprintf(stderr, "\n");
 
@@ -922,9 +926,9 @@ daemonUsage(const char *argv0, bool privileged)
 
     fprintf(stderr, "    %s:\n",
             _("PID file (unless overridden by -p)"));
-    fprintf(stderr, "      %s\n",
-            privileged ? LOCALSTATEDIR "/run/libvirtd.pid":
-            "$XDG_RUNTIME_DIR/libvirt/libvirtd.pid");
+    fprintf(stderr, "      %s/\n",
+            privileged ? LOCALSTATEDIR "/run/" DAEMON_NAME ".pid":
+            "$XDG_RUNTIME_DIR/libvirt/" DAEMON_NAME ".pid");
     fprintf(stderr, "\n");
 }
 
@@ -1088,7 +1092,7 @@ int main(int argc, char **argv) {
     if (!pid_file &&
         virPidFileConstructPath(privileged,
                                 LOCALSTATEDIR,
-                                "libvirtd",
+                                DAEMON_NAME,
                                 &pid_file) < 0) {
         VIR_ERROR(_("Can't determine pid file path."));
         exit(EXIT_FAILURE);
@@ -1168,7 +1172,7 @@ int main(int argc, char **argv) {
         goto cleanup;
     }
 
-    if (!(srv = virNetServerNew("libvirtd", 1,
+    if (!(srv = virNetServerNew(DAEMON_NAME, 1,
                                 config->min_workers,
                                 config->max_workers,
                                 config->prio_workers,
diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c
index 537b90a855..3e62b4203f 100644
--- a/src/remote/remote_daemon_config.c
+++ b/src/remote/remote_daemon_config.c
@@ -77,7 +77,8 @@ int
 daemonConfigFilePath(bool privileged, char **configfile)
 {
     if (privileged) {
-        if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/libvirtd.conf") < 0)
+        if (VIR_STRDUP(*configfile,
+                       SYSCONFDIR "/libvirt/" DAEMON_NAME ".conf") < 0)
             goto error;
     } else {
         char *configdir = NULL;
@@ -85,7 +86,7 @@ daemonConfigFilePath(bool privileged, char **configfile)
         if (!(configdir = virGetUserConfigDirectory()))
             goto error;
 
-        if (virAsprintf(configfile, "%s/libvirtd.conf", configdir) < 0) {
+        if (virAsprintf(configfile, "%s/%s.conf", configdir, DAEMON_NAME) < 0) {
             VIR_FREE(configdir);
             goto error;
         }
diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h
index 8c7da6b000..132e478ef3 100644
--- a/src/remote/remote_driver.h
+++ b/src/remote/remote_driver.h
@@ -34,7 +34,6 @@ unsigned long remoteVersion(void);
 #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock"
 #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro"
 #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock"
-#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf"
 
 /* Defaults for PKI directory. */
 #define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
Posted by Andrea Bolognani 6 years, 6 months ago
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> @@ -895,7 +899,7 @@ daemonUsage(const char *argv0, bool privileged)
>      fprintf(stderr, "\n");
>  
>      fprintf(stderr, "    %s:\n", _("Configuration file (unless overridden by -f)"));
> -    fprintf(stderr, "      %s/libvirt/libvirtd.conf\n",
> +    fprintf(stderr, "      %s/libvirt/" DAEMON_NAME ".conf\n",
>              privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");

Similarly to the previous commit, this should be

  fprintf(stderr, "      %s/libvirt/%s.conf\n",
          DAEMON_NAME, privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");

[...]
> @@ -922,9 +926,9 @@ daemonUsage(const char *argv0, bool privileged)
>  
>      fprintf(stderr, "    %s:\n",
>              _("PID file (unless overridden by -p)"));
> -    fprintf(stderr, "      %s\n",
> -            privileged ? LOCALSTATEDIR "/run/libvirtd.pid":
> -            "$XDG_RUNTIME_DIR/libvirt/libvirtd.pid");
> +    fprintf(stderr, "      %s/\n",
> +            privileged ? LOCALSTATEDIR "/run/" DAEMON_NAME ".pid":
> +            "$XDG_RUNTIME_DIR/libvirt/" DAEMON_NAME ".pid");

The pattern suggested above and in the previous patch make even more
sense here.

[...]
> +++ b/src/remote/remote_daemon_config.c
> @@ -77,7 +77,8 @@ int
>  daemonConfigFilePath(bool privileged, char **configfile)
>  {
>      if (privileged) {
> -        if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/libvirtd.conf") < 0)
> +        if (VIR_STRDUP(*configfile,
> +                       SYSCONFDIR "/libvirt/" DAEMON_NAME ".conf") < 0)

Same here - just use virAsprintf() instead of VIR_STRDUP().

[...]
> @@ -85,7 +86,7 @@ daemonConfigFilePath(bool privileged, char **configfile)
>          if (!(configdir = virGetUserConfigDirectory()))
>              goto error;
>  
> -        if (virAsprintf(configfile, "%s/libvirtd.conf", configdir) < 0) {
> +        if (virAsprintf(configfile, "%s/%s.conf", configdir, DAEMON_NAME) < 0) {

This is what I'm talking about! ;)

[...]
> +++ b/src/remote/remote_driver.h
> @@ -34,7 +34,6 @@ unsigned long remoteVersion(void);
>  #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock"
>  #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro"
>  #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock"
> -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf"

Oh, this was unused even before your changes, wasn't it? You should
drop it in a separate, trivial patch.


Going through this patch only strenghtened my convintion that what I
suggested in the previous patch is the way to go, so I urge you to
implement that suggestion; however, for the sake of being coherent,
even if you decide not to go through with it you can still consider
this

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
Posted by Daniel P. Berrangé 6 years, 6 months ago
On Fri, Jul 26, 2019 at 02:39:51PM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
> [...]
> > +++ b/src/remote/remote_driver.h
> > @@ -34,7 +34,6 @@ unsigned long remoteVersion(void);
> >  #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock"
> >  #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro"
> >  #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock"
> > -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf"
> 
> Oh, this was unused even before your changes, wasn't it? You should
> drop it in a separate, trivial patch.

No it was used, but this should have been done in patch 6 instead.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
Posted by Andrea Bolognani 6 years, 6 months ago
On Fri, 2019-07-26 at 16:24 +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 26, 2019 at 02:39:51PM +0200, Andrea Bolognani wrote:
> > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
> > [...]
> > > +++ b/src/remote/remote_driver.h
> > > @@ -34,7 +34,6 @@ unsigned long remoteVersion(void);
> > >  #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock"
> > >  #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro"
> > >  #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock"
> > > -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf"
> > 
> > Oh, this was unused even before your changes, wasn't it? You should
> > drop it in a separate, trivial patch.
> 
> No it was used, but this should have been done in patch 6 instead.

Right you are! I just checked @^ instead of master... The hunk should
be squashed into patch 6 then.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list