[libvirt] [PATCH v3 07/48] remote: conditionalize socket names in libvirtd daemon

[Christophe de Dinechin]

(Sorry if this is a resend, my earlier email fired up too quickly)


Daniel P. Berrangé writes:

> Prepare for reusing libvirtd source to create other daemons by making
> the socket names conditionally defined by the make rules.
>
> Reviewed-by: Andrea Bolognani <abologna at redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/remote/Makefile.inc.am |  1 +
>  src/remote/remote_daemon.c | 34 +++++++++++++++++++++-------------
>  2 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
> index 0400dabad9..ced940d3c1 100644
> --- a/src/remote/Makefile.inc.am
> +++ b/src/remote/Makefile.inc.am
> @@ -146,6 +146,7 @@ libvirtd_CFLAGS = \
>  	-I$(srcdir)/access \
>  	-I$(srcdir)/conf \
>  	-I$(srcdir)/rpc \
> +	-DSOCK_PREFIX="\"libvirt\"" \
>  	$(NULL)
>
>  libvirtd_LDFLAGS = \
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 69385af1c4..f9d923b357 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -221,19 +221,25 @@ daemonUnixSocketPaths(struct daemonConfig *config,
>      char *rundir = NULL;
>
>      if (config->unix_sock_dir) {
> -        if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir) < 0)
> +        if (virAsprintf(sockfile, "%s/%s-sock",
> +                        SOCK_PREFIX, config->unix_sock_dir) < 0)
>              goto cleanup;
>
>          if (privileged) {
> -            if (virAsprintf(rosockfile, "%s/libvirt-sock-ro", config->unix_sock_dir) < 0 ||
> -                virAsprintf(admsockfile, "%s/libvirt-admin-sock", config->unix_sock_dir) < 0)
> +            if (virAsprintf(rosockfile, "%s/%s-sock-ro",
> +                            SOCK_PREFIX, config->unix_sock_dir) < 0 ||
> +                virAsprintf(admsockfile, "%s/%s-admin-sock",
> +                            SOCK_PREFIX, config->unix_sock_dir) < 0)
>                  goto cleanup;
>          }
>      } else {
>          if (privileged) {
> -            if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock") < 0 ||
> -                VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro") < 0 ||
> -                VIR_STRDUP(*admsockfile, LOCALSTATEDIR "/run/libvirt/libvirt-admin-sock") < 0)
> +            if (virAsprintf(sockfile, "%s/run/libvirt/%s-sock",
> +                            LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +                virAsprintf(sockfile, "%s/run/libvirt/%s-sock-ro",
> +                            LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +                virAsprintf(sockfile, "%s/run/libvirt/%s-admin-sock",
> +                            LOCALSTATEDIR, SOCK_PREFIX) < 0)

Copy-paste error on sockfile variable name, use rosockfile and admsockfile.

Also, there is a memory leak if second or third fails, since the first
one is never deallocated.

Consider adding a VIR_FREE for *sockfile, *rosockfile and *admsockfile
in the cleanup section. Also, to make it real safe, consider adding a
NULL-initialization for *sockfile, *rosockfile an d *admsockfile at the
top of the function.


>                  goto cleanup;
>          } else {
>              mode_t old_umask;
> @@ -248,8 +254,10 @@ daemonUnixSocketPaths(struct daemonConfig *config,
>              }
>              umask(old_umask);
>
> -            if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 ||
> -                virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) < 0)
> +            if (virAsprintf(sockfile, "%s/%s-sock",
> +                            rundir, SOCK_PREFIX) < 0 ||
> +                virAsprintf(admsockfile, "%s/%s-admin-sock",
> +                            rundir, SOCK_PREFIX) < 0)
>                  goto cleanup;
>          }
>      }
> @@ -902,12 +910,12 @@ daemonUsage(const char *argv0, bool privileged)
>      fprintf(stderr, "\n");
>
>      fprintf(stderr, "    %s:\n", _("Sockets"));

Localization of :

> -    fprintf(stderr, "      %s\n",
> -            privileged ? LOCALSTATEDIR "/run/libvirt/libvirt-sock" :
> -            "$XDG_RUNTIME_DIR/libvirt/libvirt-sock");
> +    fprintf(stderr, "      %s/libvirt/%s-sock\n",
> +            privileged ? LOCALSTATEDIR "/run" : "$XDG_RUNTIME_DIR",
> +            SOCK_PREFIX);
>      if (privileged)
> -        fprintf(stderr, "      %s\n",
> -                LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro");
> +        fprintf(stderr, "      %s/run/libvirt/%s-sock-ro\n",
> +                LOCALSTATEDIR, SOCK_PREFIX);
>      fprintf(stderr, "\n");
>
>      fprintf(stderr, "    %s:\n", _("TLS"));

Localization of :

> --
> 2.21.0

Reviewed-by: Christophe de Dinechin <dinechin at redhat.com>

--
Cheers,
Christophe de Dinechin (IRC c3d)