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

On Mon, Nov 30, 2015 at 09:35:39AM +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. > In order to fix this also for domains that already exist, we should ignore any socket paths starting with /var/lib/libvirt as those were generated and users shouldn't explicitly specify them anyway.

>Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >--- > src/conf/domain_conf.c | 2 +- > src/qemu/qemu_command.c | 28 ++++++++++++++++++++++++++++ > src/qemu/qemu_command.h | 1 + > src/qemu/qemu_domain.c | 16 ---------------- > src/qemu/qemu_driver.c | 10 ++++++++++ > src/qemu/qemu_process.c | 3 +++ > tests/qemuhotplugtest.c | 5 +++++ > tests/qemuxml2argvtest.c | 5 +++++ > 8 files changed, 53 insertions(+), 17 deletions(-) > >diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >index cbfc41e..ebae828 100644 >--- a/src/conf/domain_conf.c >+++ b/src/conf/domain_conf.c >@@ -9810,7 +9810,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, > goto error; > } > >- def->data.nix.listen = mode != NULL && STRNEQ(mode, "connect"); >+ def->data.nix.listen = mode == NULL || STRNEQ(mode, "connect"); > This changes the default unix socket mode from 'connect' to 'bind', is this just a leftover from some testing or is there a reason behind that. We have no default documented anywhere, so it wouldn't be an issue, I guess.

> def->data.nix.path = path; > path = NULL; >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; >- } >- > /* forbid capabilities mode hostdev in this kind of hypervisor */ > if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && > dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { >diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >index ae1d8e7..b4fac55 100644 >--- a/src/qemu/qemu_driver.c >+++ b/src/qemu/qemu_driver.c >@@ -7254,6 +7254,9 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, > if (qemuAssignDeviceAliases(def, qemuCaps) < 0) > goto cleanup; > >+ if (qemuUnixSocketGenerate(def, cfg) < 0) >+ goto cleanup; >+ > if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) > goto cleanup; > >@@ -15846,6 +15849,7 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, > unsigned int flags) > { > virQEMUDriverPtr driver = conn->privateData; >+ virQEMUDriverConfigPtr cfg = NULL; > virDomainObjPtr vm = NULL; > virDomainDefPtr def = NULL; > virDomainPtr dom = NULL; >@@ -15858,6 +15862,8 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, > > virCheckFlags(0, NULL); > >+ cfg = virQEMUDriverGetConfig(driver); >+ > if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > goto cleanup; > >@@ -15895,6 +15901,9 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, > if (qemuAssignDeviceAliases(def, qemuCaps) < 0) > goto cleanup; > >+ if (qemuUnixSocketGenerate(def, cfg) < 0) >+ goto cleanup; >+ Why would we generate some path for a qemu that's already running and we are not passing that path to qemu? It would have no effect and only provide false information to the user.

> if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) > goto cleanup; > >@@ -15936,6 +15945,7 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, > VIR_FREE(pidfile); > virObjectUnref(caps); > virObjectUnref(qemuCaps); >+ virObjectUnref(cfg); > return dom; > } > >diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >index 192730c..29cf965 100644 >--- a/src/qemu/qemu_process.c >+++ b/src/qemu/qemu_process.c >@@ -4663,6 +4663,9 @@ qemuProcessLaunch(virConnectPtr conn, > if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) > goto cleanup; > >+ if (qemuUnixSocketGenerate(vm->def, cfg) < 0) >+ goto cleanup; >+ If you move it from here to qemuBuildCommandLine(), then you don't need to mangle with tests. It sounds weird that we don't have a clear split between completing the info in XML and building a command-line, but that was never the case anyway.

> /* Get the advisory nodeset from numad if 'placement' of > * either <vcpu> or <numatune> is 'auto'. > */ >diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c >index 102e052..b17cca2 100644 >--- a/tests/qemuhotplugtest.c >+++ b/tests/qemuhotplugtest.c >@@ -61,6 +61,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, > { > int ret = -1; > qemuDomainObjPrivatePtr priv = NULL; >+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(&driver); > > if (!(*vm = virDomainObjNew(xmlopt))) > goto cleanup; >@@ -94,10 +95,14 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, > if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0) > goto cleanup; > >+ if (qemuUnixSocketGenerate((*vm)->def, cfg) < 0) >+ goto cleanup; >+ > (*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID; > > ret = 0; > cleanup: >+ virObjectUnref(cfg); > return ret; > } > >diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >index f7596a0..f6083db 100644 >--- a/tests/qemuxml2argvtest.c >+++ b/tests/qemuxml2argvtest.c >@@ -262,6 +262,7 @@ static int testCompareXMLToArgvFiles(const char *xml, > virCommandPtr cmd = NULL; > size_t i; > virBitmapPtr nodeset = NULL; >+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(&driver); > > if (!(conn = virGetConnect())) > goto out; >@@ -324,6 +325,9 @@ static int testCompareXMLToArgvFiles(const char *xml, > if (qemuAssignDeviceAliases(vmdef, extraFlags) < 0) > goto out; > >+ if (qemuUnixSocketGenerate(vmdef, cfg) < 0) >+ goto out; >+ > for (i = 0; i < vmdef->nhostdevs; i++) { > virDomainHostdevDefPtr hostdev = vmdef->hostdevs[i]; > >@@ -387,6 +391,7 @@ static int testCompareXMLToArgvFiles(const char *xml, > virCommandFree(cmd); > virDomainDefFree(vmdef); > virObjectUnref(conn); >+ virObjectUnref(cfg); > virBitmapFree(nodeset); > return ret; > } >-- >2.6.3 > >-- >libvir-list mailing list &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list