2:53 a.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/$domain.agent
---
This makes the qemu agent to be generated at domain startup phase.
So until that, we generate XML without any path, e.g.:
<channel type='unix'>
<source mode='connect'/>
<target type='virtio' name='org.qemu.guest_agent.0'/>
<address type='virtio-serial' controller='0' bus='0'
port='1'/>
</channel>
The other possibility is to generate path during XML parse phase, however this
expose something we are lacking for years - callbacks to fill in default values
for not configured ones. The aim so to make libvirt accept this snippet as
valid qemu agent config:
<channel type='unix'>
<target type='virtio' name='org.qemu.guest_agent.0'/>
</channel>
src/conf/domain_conf.c | 37 ++++++++++++++++++++-----------------
src/qemu/qemu_process.c | 30 ++++++++++++++++++++++++++++--
2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7a2b012..6245fec 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6038,7 +6038,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
- if (path == NULL) {
+ /* path is not required in special case of qemu guest agent */
+ if (path == NULL &&
+ chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO
&&
+ STRNEQ(chr_def->target.name, "org.qemu.guest_agent.0")) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing source path attribute for char
device"));
goto error;
@@ -6135,7 +6138,6 @@ virDomainChrDefParseXML(virCapsPtr caps,
char *type = NULL;
const char *nodeName;
virDomainChrDefPtr def;
- int remaining;
bool seenTarget = false;
if (!(def = virDomainChrDefNew()))
@@ -6159,29 +6161,28 @@ virDomainChrDefParseXML(virCapsPtr caps,
}
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(caps, vmdef, 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(caps, vmdef, def, cur) < 0) {
+ goto error;
}
+ break;
}
- cur = cur->next;
}
+ cur = cur->next;
}
if (!seenTarget &&
((def->targetType = virDomainChrDefaultTargetType(caps, vmdef,
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",
@@ -13307,10 +13308,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
+
virBufferAsprintf(buf, " <source mode='%s'",
def->data.nix.listen ? "bind" :
"connect");
- virBufferEscapeString(buf, " path='%s'/>\n",
+ virBufferEscapeString(buf, " path='%s'",
def->data.nix.path);
+ virBufferAddLit(buf, "/>\n");
break;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4251c34..90f072d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2788,6 +2788,31 @@ qemuProcessPrepareMonitorChr(virQEMUDriverConfigPtr cfg,
return 0;
}
+static int
+qemuProcessPrepareAgentChr(virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr vm)
+{
+ virDomainChrSourceDefPtr config = qemuFindAgentConfig(vm->def);
+ virDomainChrSourceDefPtr newConfing = qemuFindAgentConfig(vm->newDef);
+
+ if (!config)
+ return 0;
+
+ /* if user hasn't supplied any agent socket path, generate one,
+ * and store it in inactive config as well */
+ if ((config->type == VIR_DOMAIN_CHR_TYPE_UNIX) &&
+ !config->data.nix.path) {
+ if ((virAsprintf(&config->data.nix.path, "%s/%s.agent",
+ cfg->libDir, vm->def->name) < 0) ||
+ !(newConfing->data.nix.path = strdup(config->data.nix.path))) {
+ virReportOOMError();
+ return -1;
+ }
+ newConfing->data.nix.listen = config->data.nix.listen = true;
+ }
+
+ return 0;
+}
/*
* Precondition: vm must be locked, and a job must be active.
@@ -3772,8 +3797,9 @@ int qemuProcessStart(virConnectPtr conn,
goto cleanup;
}
- VIR_DEBUG("Preparing monitor state");
- if (qemuProcessPrepareMonitorChr(cfg, priv->monConfig, vm->def->name) <
0)
+ VIR_DEBUG("Preparing monitor and agent state");
+ if ((qemuProcessPrepareMonitorChr(cfg, priv->monConfig, vm->def->name) <
0) ||
+ (qemuProcessPrepareAgentChr(cfg, vm) < 0))
goto cleanup;
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON))
--
1.8.0.2
4:58 a.m.
On Fri, Feb 15, 2013 at 09:53:17AM +0100, 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/$domain.agent
IMHO the name should be postfixed by the "channel/target/@name" value
not merely 'agent', otherwise you're introducing uniqueness problems.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:56 a.m.
On 02/15/13 09:53, 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/$domain.agent
---
This makes the qemu agent to be generated at domain startup phase.
So until that, we generate XML without any path, e.g.:
<channel type='unix'>
<source mode='connect'/>
<target type='virtio' name='org.qemu.guest_agent.0'/>
<address type='virtio-serial' controller='0' bus='0'
port='1'/>
</channel>
The other possibility is to generate path during XML parse phase, however this
expose something we are lacking for years - callbacks to fill in default values
Yep, that is missing. Without that you will fill the path only when the
machine is started. This isn't ideal. I have the same issue with
recording the default network model in the XML.
for not configured ones. The aim so to make libvirt accept this
snippet as
valid qemu agent config:
<channel type='unix'>
<target type='virtio' name='org.qemu.guest_agent.0'/>
</channel>
src/conf/domain_conf.c | 37 ++++++++++++++++++++-----------------
src/qemu/qemu_process.c | 30 ++++++++++++++++++++++++++++--
2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7a2b012..6245fec 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6038,7 +6038,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
- if (path == NULL) {
+ /* path is not required in special case of qemu guest agent */
+ if (path == NULL &&
+ chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO
&&
+ STRNEQ(chr_def->target.name, "org.qemu.guest_agent.0")) {
Hm. Not entirely elegant, but this is a corner case, so it's not worth
refactoring other use of the unix chardev backend.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing source path attribute for char
device"));
goto error;
@@ -6135,7 +6138,6 @@ virDomainChrDefParseXML(virCapsPtr caps,
char *type = NULL;
const char *nodeName;
virDomainChrDefPtr def;
- int remaining;
bool seenTarget = false;
if (!(def = virDomainChrDefNew()))
@@ -13307,10 +13308,12 @@ virDomainChrSourceDefFormat(virBufferPtr
buf,
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
+
This newline isn't present in the other "case"s. Don't add it.
virBufferAsprintf(buf, " <source
mode='%s'",
def->data.nix.listen ? "bind" :
"connect");
- virBufferEscapeString(buf, " path='%s'/>\n",
+ virBufferEscapeString(buf, " path='%s'",
def->data.nix.path);
+ virBufferAddLit(buf, "/>\n");
break;
}
11:30 a.m.
On 02/15/2013 06:56 AM, Peter Krempa wrote:
On 02/15/13 09:53, Michal Privoznik wrote:
> The other possibility is to generate path during XML parse phase,
> however this
> expose something we are lacking for years - callbacks to fill in
> default values
Yep, that is missing. Without that you will fill the path only when
the machine is started. This isn't ideal. I have the same issue with
recording the default network model in the XML.
There are many things with defaults filled in by the parser (either
blindly, or after checking for the type of domain) that IMO really
should be determined by the hypervisor. The biggest example of this is
pretty much everything added by virDomainDefAddImplicitControllers().
Ideally, *nothing* should be added by a call to virDomainDefParseXML();
it should simply build an exact representation of the XML document in
the form of C objects. Since any defaults/implicit devices/whatever
depend not only on the target hypervisor, but often also on the
machinetype, all of that should be done after return from the parse (is
there an instance where it's necessary to add the extra complexity of a
callback?)
2:32 p.m.
On 02/15/13 18:30, Laine Stump wrote:
On 02/15/2013 06:56 AM, Peter Krempa wrote:
> On 02/15/13 09:53, Michal Privoznik wrote:
>> The other possibility is to generate path during XML parse phase,
>> however this
>> expose something we are lacking for years - callbacks to fill in
>> default values
>
> Yep, that is missing. Without that you will fill the path only when
> the machine is started. This isn't ideal. I have the same issue with
> recording the default network model in the XML.
There are many things with defaults filled in by the parser (either
blindly, or after checking for the type of domain) that IMO really
should be determined by the hypervisor. The biggest example of this is
pretty much everything added by virDomainDefAddImplicitControllers().
Ideally, *nothing* should be added by a call to virDomainDefParseXML();
it should simply build an exact representation of the XML document in
the form of C objects. Since any defaults/implicit devices/whatever
depend not only on the target hypervisor, but often also on the
machinetype, all of that should be done after return from the parse (is
there an instance where it's necessary to add the extra complexity of a
callback?)
The callback is a idea that me, Michal and Jiri were discussing earlier.
It's meant as a abstraction of what you said. The XML is parsed on many
places and the callback would eliminate the need to add the filling
function after every call of the parser.
I agree that the parser shouldn't fill any defaults as it will sooner or
later bite us somewhere else. As of the machine type and controllers:
q35 is biting you, isn't it? :)
As of adding the callback: it might be better to just add a filler
function after every call of the parser, but I was thinking of something
more lazy :)
Peter
2:47 p.m.
On 02/15/2013 01:32 PM, Peter Krempa wrote:
The callback is a idea that me, Michal and Jiri were discussing earlier.
It's meant as a abstraction of what you said. The XML is parsed on many
places and the callback would eliminate the need to add the filling
function after every call of the parser.
Furthermore, we are already passing a virCapsPtr capability object
through to much of the parser - that capability object would be a
natural place to add whatever callback function pointers are needed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:54 p.m.
On 02/15/2013 03:47 PM, Eric Blake wrote:
On 02/15/2013 01:32 PM, Peter Krempa wrote:
> The callback is a idea that me, Michal and Jiri were discussing earlier.
> It's meant as a abstraction of what you said. The XML is parsed on many
> places and the callback would eliminate the need to add the filling
> function after every call of the parser.
Furthermore, we are already passing a virCapsPtr capability object
through to much of the parser - that capability object would be a
natural place to add whatever callback function pointers are needed.
Ah, I hadn't realized the extent of virCaps usage outside of qemu. It
does look like a good fit, possibly even for a *set* of callback
functions (I haven't thought about exactly what they might be).
4298
days inactive
4299
days old
6 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Michal Privoznik -
Peter Krempa