[libvirt] [PATCH] conf: 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/$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
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 :|
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; }
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?)
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
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
On 02/15/2013 03:47 PM, Eric Blake 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
On 02/15/2013 01:32 PM, Peter Krempa wrote: 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).
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Michal Privoznik -
Peter Krempa