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

[Martin Kletzander]

On Mon, Aug 24, 2015 at 10:32:03AM +0100, Daniel P. Berrange wrote:
>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.
>

To be honest, this worried me a *lot* actually.  But since it already
existed and I spent way too much trying to fix this as cleanly as
possible, I rather posted it as is thinking the porblem might be dealt
with later on, although I'm not quite sure it's worth the code change.

Thanks for the review.

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