[libvirt] [PATCH v2 7/9] qemu: Fix access to auto-generated socket paths

[Daniel P. Berrange]

On Mon, Aug 17, 2015 at 12:16:48PM -0700, Martin Kletzander wrote:
> We are automatically generating some socket paths for domains, but all
> those paths end up in a directory that's the same for multiple domains.
> The problem is that multiple domains can each run with different
> seclabels (users, selinux contexts, etc.).  The idea here is to create a
> per-domain directory labelled in a way that each domain can access its
> own unix sockets.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146886
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  src/qemu/qemu_command.c                            |  2 +-
>  src/qemu/qemu_domain.c                             | 16 +++---
>  src/qemu/qemu_process.c                            | 57 +++++++++++++++++++++-
>  .../qemuxml2argv-channel-virtio-unix.args          |  7 +--
>  4 files changed, 67 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c5a4fdf09a2b..abc57d762075 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8051,7 +8051,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>      if (graphics->data.vnc.socket || cfg->vncAutoUnixSocket) {
>          if (!graphics->data.vnc.socket &&
>              virAsprintf(&graphics->data.vnc.socket,
> -                        "%s/%s.vnc", cfg->libDir, def->name) == -1)
> +                        "%s/domain-%s/vnc.sock", cfg->libDir, def->name) == -1)
>              goto error;
> 
>          virBufferAsprintf(&opt, "unix:%s", graphics->data.vnc.socket);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 84e5fa530cba..0a9ed6babb4c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1307,16 +1307,12 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>              goto cleanup;
>          }
> 
> -        if (dev->data.chr->target.name) {
> -            if (virAsprintf(&dev->data.chr->source.data.nix.path, "%s/%s.%s",
> -                            cfg->channelTargetDir,
> -                            def->name, dev->data.chr->target.name) < 0)
> -                goto cleanup;
> -        } else {
> -            if (virAsprintf(&dev->data.chr->source.data.nix.path, "%s/%s",
> -                            cfg->channelTargetDir, def->name) < 0)
> -                goto cleanup;
> -        }
> +        if (virAsprintf(&dev->data.chr->source.data.nix.path,
> +                        "%s/domain-%s/%s",
> +                        cfg->channelTargetDir, def->name,
> +                        dev->data.chr->target.name ? dev->data.chr->target.name
> +                        : "unknown.sock") < 0)
> +            goto cleanup;

This worries me a little - IIUC we could end up with multiple devices
using the same "unknown.sock" file path. If I'm reading correctly, the
original code had this problem too. Would we be justified in raising
an error in this scenario ?

ACK anyway, since it appears the problem already existed.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|