[libvirt] [PATCH] qemu: Generate agent socket path if missing
It's not desired to force users imagine path for a socket they are not even supposed to connect to. On the other hand, we already have a release where the qemu agent socket path is exposed to XML, so we cannot silently drop it from there. The new path is generated in form: $LOCALSTATEDIR/lib/libvirt/qemu/channel/target/$domain.$name --- libvirt.spec.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 34 ++++++++++++++++------------------ src/qemu/qemu_domain.c | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 328b009..11fc12d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1719,6 +1719,7 @@ fi %if %{with_qemu} %ghost %dir %attr(0700, root, root) %{_localstatedir}/run/libvirt/qemu/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/ +%dir %attr{0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/target/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/ %endif %if %{with_lxc} diff --git a/src/Makefile.am b/src/Makefile.am index fc6b846..9576da8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2097,6 +2097,7 @@ if WITH_SANLOCK endif if WITH_QEMU $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu" + $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu/channel/target" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/qemu" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt/qemu" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/log/libvirt/qemu" diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03e5740..3ff548a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6464,7 +6464,9 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - if (path == NULL) { + /* path can be auto generated */ + if (!path && + chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing source path attribute for char device")); goto error; @@ -6559,7 +6561,6 @@ virDomainChrDefParseXML(xmlXPathContextPtr ctxt, char *type = NULL; const char *nodeName; virDomainChrDefPtr def; - int remaining; bool seenTarget = false; if (!(def = virDomainChrDefNew())) @@ -6583,29 +6584,26 @@ virDomainChrDefParseXML(xmlXPathContextPtr ctxt, } cur = node->children; - remaining = virDomainChrSourceDefParseXML(&def->source, cur, flags, - def, ctxt, - vmSeclabels, nvmSeclabels); - if (remaining < 0) - goto error; - if (remaining) { - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(cur->name, BAD_CAST "target")) { - seenTarget = true; - if (virDomainChrDefParseTargetXML(def, cur) < 0) { - goto error; - } + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "target")) { + seenTarget = true; + if (virDomainChrDefParseTargetXML(def, cur) < 0) { + goto error; } } - cur = cur->next; } + cur = cur->next; } if (!seenTarget && ((def->targetType = virDomainChrDefaultTargetType(def->deviceType)) < 0)) goto cleanup; + if (virDomainChrSourceDefParseXML(&def->source, node->children, flags, def, + ctxt, vmSeclabels, nvmSeclabels) < 0) + goto error; + if (def->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { if (def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -13897,8 +13895,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_CHR_TYPE_UNIX: virBufferAsprintf(buf, " <source mode='%s'", def->data.nix.listen ? "bind" : "connect"); - virBufferEscapeString(buf, " path='%s'/>\n", - def->data.nix.path); + virBufferEscapeString(buf, " path='%s'", def->data.nix.path); + virBufferAddLit(buf, "/>\n"); break; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d6eef4..967890f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -738,6 +738,22 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X)) dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO; + /* 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 && + (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver))))) { + + if (virAsprintf(&dev->data.chr->source.data.nix.path, + "%s/channel/target/%s.%s", + cfg->libDir, def->name, + dev->data.chr->target.name) < 0) + goto no_memory; + dev->data.chr->source.data.nix.listen = true; + } + ret = 0; cleanup: -- 1.8.1.5
On Tue, Apr 09, 2013 at 19:05:28 +0200, Michal Privoznik wrote:
It's not desired to force users imagine path for a socket they are not even supposed to connect to. On the other hand, we already have a release where the qemu agent socket path is exposed to XML, so we cannot silently drop it from there. The new path is generated in form:
$LOCALSTATEDIR/lib/libvirt/qemu/channel/target/$domain.$name --- libvirt.spec.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 34 ++++++++++++++++------------------ src/qemu/qemu_domain.c | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 18 deletions(-) ... diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d6eef4..967890f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -738,6 +738,22 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X)) dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
+ /* 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 && + (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver))))) {
I think this should fail if path is NULL and (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver)))) is not true. On the other hand, do we actually need to check for this? Aren't both cfg and driver guaranteed to be non-NULL at this point?
+ + if (virAsprintf(&dev->data.chr->source.data.nix.path, + "%s/channel/target/%s.%s", + cfg->libDir, def->name, + dev->data.chr->target.name) < 0) + goto no_memory; + dev->data.chr->source.data.nix.listen = true; + } + ret = 0;
cleanup:
Jirka
On 04/23/2013 09:40 AM, Jiri Denemark wrote:
On Tue, Apr 09, 2013 at 19:05:28 +0200, Michal Privoznik wrote:
It's not desired to force users imagine path for a socket they are not even supposed to connect to. On the other hand, we already have a release where the qemu agent socket path is exposed to XML, so we cannot silently drop it from there. The new path is generated in form:
$LOCALSTATEDIR/lib/libvirt/qemu/channel/target/$domain.$name ---
I like the idea of this patch, but think we probably also ought to document this choice of auto-generated path in formatdomain.html.in somehow.
libvirt.spec.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 34 ++++++++++++++++------------------ src/qemu/qemu_domain.c | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 18 deletions(-)
+ /* 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 && + (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver))))) {
I think this should fail if path is NULL and (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver)))) is not true.
On the other hand, do we actually need to check for this? Aren't both cfg and driver guaranteed to be non-NULL at this point?
'driver' corresponds to the opaque argument; we already have other examples in the function that assume it can be NULL on some callback paths: /* set default disk types and drivers */ if (dev->type == VIR_DOMAIN_DEVICE_DISK) { virDomainDiskDefPtr disk = dev->data.disk; /* both of these require data from the driver config */ if (driver && (cfg = virQEMUDriverGetConfig(driver))) { so this is just matching existing style. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 23.04.2013 17:40, Jiri Denemark wrote:
On Tue, Apr 09, 2013 at 19:05:28 +0200, Michal Privoznik wrote:
It's not desired to force users imagine path for a socket they are not even supposed to connect to. On the other hand, we already have a release where the qemu agent socket path is exposed to XML, so we cannot silently drop it from there. The new path is generated in form:
$LOCALSTATEDIR/lib/libvirt/qemu/channel/target/$domain.$name --- libvirt.spec.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 34 ++++++++++++++++------------------ src/qemu/qemu_domain.c | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 18 deletions(-) ... diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d6eef4..967890f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -738,6 +738,22 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X)) dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
+ /* auto generate unix socket path */ + if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
1: ^^
+ 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 && + (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver))))) {
I think this should fail if path is NULL and (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver)))) is not true.
On the other hand, do we actually need to check for this? Aren't both cfg and driver guaranteed to be non-NULL at this point?
And actually as Eric pointed out, driver can be NULL and so does cfg. The only way where cfg already is not NULL is for (dev->type == VIR_DOMAIN_DEVICE_DISK) in which case the [1] condition is false. So in fact, there is no way for cfg to be other than NULL, hence I can drop check for cfg being not NULL: if (dev->type = VIR_DOMAIN_DEVICE_CHR && ... (driver && (cfg = virQEMUDriverGetConfig(driver)))) {
+ + if (virAsprintf(&dev->data.chr->source.data.nix.path, + "%s/channel/target/%s.%s", + cfg->libDir, def->name, + dev->data.chr->target.name) < 0) + goto no_memory; + dev->data.chr->source.data.nix.listen = true; + } + ret = 0;
cleanup:
Jirka
Michal
participants (3)
-
Eric Blake -
Jiri Denemark -
Michal Privoznik