[libvirt] [PATCH] guest-agent-socket: don't generate default path to config XML
Pavel Hrdina
phrdina at redhat.com
Mon Nov 30 11:18:41 UTC 2015
On Mon, Nov 30, 2015 at 10:21:47AM +0100, Martin Kletzander wrote:
> 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.
This is true only for domains created by system connection, for session
connection it's $HOME/.local/libvirt/..., the question is, whether we care about
session connection or not?
>
> >Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> >---
> > 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.
Interesting, I don't event know why I've changed it and it seems, that this
change is not required. I'll remove it.
>
> > 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.
That's true, I'll remove it.
>
> > 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.
I've thought about that, but than I've decide that this move would be better in
separate patch with all other function, that should be moved.
>
> > /* 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
> >libvir-list at redhat.com
> >https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list