[libvirt] [PATCH v2] guest-agent-socket: don't generate default path to config XML

[Martin Kletzander]

On Mon, Nov 30, 2015 at 12:37:58PM +0100, Pavel Hrdina wrote:
>While we started using for all unix sockets as default one common
>directory based on a guest name it introduced several issues, for example
>with renaming the guest or cloning it.  In general it's not entirely
>bad, but in this case it would be best to hide the auto-generated socket
>path from user and don't export it in the config XML.
>
>Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>---
>
>Version 2:
>    - removed unnecessary changes
>
> src/qemu/qemu_command.c  | 28 ++++++++++++++++++++++++++++
> src/qemu/qemu_command.h  |  1 +
> src/qemu/qemu_domain.c   | 16 ----------------
> src/qemu/qemu_driver.c   |  3 +++
> src/qemu/qemu_process.c  |  3 +++
> tests/qemuhotplugtest.c  |  5 +++++
> tests/qemuxml2argvtest.c |  5 +++++
> 7 files changed, 45 insertions(+), 16 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 2a9fab5..f1bb621 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -1262,6 +1262,34 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> }
>
>
>+int
>+qemuUnixSocketGenerate(virDomainDefPtr def,
>+                       virQEMUDriverConfigPtr cfg)
>+{
>+    size_t i;
>+
>+    for (i = 0; i < def->nchannels; i++) {
>+        virDomainChrDefPtr channel = def->channels[i];
>+
>+        if (channel->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>+            channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
>+            channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX &&
>+            !channel->source.data.nix.path) {
>+            if (virAsprintf(&channel->source.data.nix.path,
>+                            "%s/domain-%s/%s",
>+                            cfg->channelTargetDir, def->name,
>+                            channel->target.name ? channel->target.name
>+                            : "unknown.sock") < 0)
>+                return -1;
>+
>+            channel->source.data.nix.listen = true;
>+        }
>+    }
>+
>+    return 0;
>+}
>+
>+
> static void
> qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>                                      virDomainDeviceAddressType type)
>diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
>index bebdd27..0512a23 100644
>--- a/src/qemu/qemu_command.h
>+++ b/src/qemu/qemu_command.h
>@@ -283,6 +283,7 @@ int qemuAssignDevicePCISlots(virDomainDefPtr def,
>                              virDomainPCIAddressSetPtr addrs);
>
> int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
>+int qemuUnixSocketGenerate(virDomainDefPtr def, virQEMUDriverConfigPtr cfg);
> int qemuDomainNetVLAN(virDomainNetDefPtr def);
> int qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx);
> int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index ed21245..7e05289 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -1329,22 +1329,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>         ARCH_IS_S390(def->os.arch))
>         dev->data.controller->model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
>
>-    /* auto generate unix socket path */
>-    if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
>-        dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>-        dev->data.chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
>-        dev->data.chr->source.type == VIR_DOMAIN_CHR_TYPE_UNIX &&
>-        !dev->data.chr->source.data.nix.path) {
>-        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;
>-
>-        dev->data.chr->source.data.nix.listen = true;
>-    }
>-

For the sake of upgradibility, here we should disregard any paths that
start with cfg->channelTargetDir, maybe even with cfg->configBaseDir.
Users should not be using paths in libvirt's directories if they need to
access it anyway.  Of course we'll need to document that as well.

I just wonder how come we don't need any tests fixed.  We should've been
testing the auto-generation, I believe.  And even if not, we should test
that we don't generate it into the XML now.

Otherwise looks fine, weak ACK due to tests, but I believe they can be
part of the next patch for the removal of these paths.
-------------- 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/20151209/a7942556/attachment-0001.sig>