12:05 p.m.
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
10:40 a.m.
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
6:41 p.m.
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
3:32 a.m.
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
4230
days inactive
4245
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Michal Privoznik