[libvirt] [PATCH 0/8] Normalization API
A while ago I've invented this vshCompleteXMLFromDomain() function to increase device-detach intelligence. Basically, it took user's XML and tried to find matching device in domain's XML. However, it was kind of buggy - finding the matching device uses string comparison. This works on text values. It doesn't on integer ones (who would expect that). So prior the lookup process, we need to normalize the integer values (and whole XML). However, this can't be done on the client side, since he has no knowledge which values are integer and which are not. Therefore we need a new API. Michal Privoznik (8): domain_conf: Introduce virDomainDeviceDefFormat Introduce new virDomainNormalizeXML API remote_driver: Implement virDomainNormalizeXML virsh: Expose virDomainNormalizeXML qemu: Implement vimDomainNormalizeXML domain_conf: Move MAC generation to post parse callback virDomainDeviceDefParse: Make PostParse callback call optional virsh: Resurrect vshCompleteXMLFromDomain daemon/remote.c | 50 ++++++++ include/libvirt/libvirt.h.in | 5 + python/generator.py | 1 + python/libvirt-override-api.xml | 7 ++ python/libvirt-override.c | 30 +++++ src/conf/domain_conf.c | 180 ++++++++++++++++------------- src/conf/domain_conf.h | 7 +- src/driver.h | 7 ++ src/libvirt.c | 51 ++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 + src/libxl/libxl_driver.c | 12 +- src/lxc/lxc_driver.c | 6 +- src/openvz/openvz_conf.c | 1 + src/openvz/openvz_driver.c | 12 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_command.c | 7 +- src/qemu/qemu_driver.c | 51 +++++++- src/remote/remote_driver.c | 40 +++++++ src/remote/remote_protocol.x | 20 +++- src/uml/uml_driver.c | 4 +- src/vbox/vbox_tmpl.c | 5 +- src/vmx/vmx.c | 2 + src/xen/xend_internal.c | 6 +- src/xen/xm_internal.c | 8 +- src/xenapi/xenapi_driver.c | 1 + src/xenxs/xen_sxpr.c | 1 + src/xenxs/xen_xm.c | 1 + tests/openvzutilstest.c | 1 - tests/qemuhotplugtest.c | 3 +- tools/virsh-domain.c | 249 +++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 8 ++ 32 files changed, 666 insertions(+), 118 deletions(-) -- 1.8.1.5
While we already have virDomainDeviceDefParse() we are still missing the format counterpart. It's not the end the world for now, but the function is going to be handy in next patches. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 141 +++++++++++++++++++++++++---------------------- src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + 3 files changed, 79 insertions(+), 65 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 05c1de4..7c92e51 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18126,6 +18126,79 @@ virDomainNetFind(virDomainDefPtr def, const char *device) return net; } +char * +virDomainDeviceDefFormat(virDomainDeviceDefPtr def, + unsigned int flags) +{ + int rc = -1; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCheckFlags(DUMPXML_FLAGS, NULL); + + switch ((virDomainDeviceType) def->type) { + case VIR_DOMAIN_DEVICE_DISK: + rc = virDomainDiskDefFormat(&buf, def->data.disk, flags); + break; + case VIR_DOMAIN_DEVICE_LEASE: + rc = virDomainLeaseDefFormat(&buf, def->data.lease); + break; + case VIR_DOMAIN_DEVICE_FS: + rc = virDomainFSDefFormat(&buf, def->data.fs, flags); + break; + case VIR_DOMAIN_DEVICE_NET: + rc = virDomainNetDefFormat(&buf, def->data.net, flags); + break; + case VIR_DOMAIN_DEVICE_INPUT: + rc = virDomainInputDefFormat(&buf, def->data.input, flags); + break; + case VIR_DOMAIN_DEVICE_SOUND: + rc = virDomainSoundDefFormat(&buf, def->data.sound, flags); + break; + case VIR_DOMAIN_DEVICE_VIDEO: + rc = virDomainVideoDefFormat(&buf, def->data.video, flags); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + rc = virDomainHostdevDefFormat(&buf, def->data.hostdev, flags); + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + rc = virDomainWatchdogDefFormat(&buf, def->data.watchdog, flags); + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + rc = virDomainControllerDefFormat(&buf, def->data.controller, flags); + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: + rc = virDomainGraphicsDefFormat(&buf, def->data.graphics, flags); + break; + case VIR_DOMAIN_DEVICE_HUB: + rc = virDomainHubDefFormat(&buf, def->data.hub, flags); + break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + rc = virDomainRedirdevDefFormat(&buf, def->data.redirdev, flags); + break; + case VIR_DOMAIN_DEVICE_RNG: + rc = virDomainRNGDefFormat(&buf, def->data.rng, flags); + break; + case VIR_DOMAIN_DEVICE_CHR: + rc = virDomainChrDefFormat(&buf, def->data.chr, flags); + break; + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Formatting XML of '%d' type " + "is not implemented yet."), + def->type); + return NULL; + } + + if (rc < 0) + return NULL; + + return virBufferContentAndReset(&buf); +} + /** * virDomainDeviceDefCopy: * @caps: Capabilities @@ -18147,76 +18220,14 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, virDomainXMLOptionPtr xmlopt) { virDomainDeviceDefPtr ret = NULL; - virBuffer buf = VIR_BUFFER_INITIALIZER; - int flags = VIR_DOMAIN_XML_INACTIVE; + unsigned int flags = VIR_DOMAIN_XML_INACTIVE; char *xmlStr = NULL; - int rc = -1; - switch ((virDomainDeviceType) src->type) { - case VIR_DOMAIN_DEVICE_DISK: - rc = virDomainDiskDefFormat(&buf, src->data.disk, flags); - break; - case VIR_DOMAIN_DEVICE_LEASE: - rc = virDomainLeaseDefFormat(&buf, src->data.lease); - break; - case VIR_DOMAIN_DEVICE_FS: - rc = virDomainFSDefFormat(&buf, src->data.fs, flags); - break; - case VIR_DOMAIN_DEVICE_NET: - rc = virDomainNetDefFormat(&buf, src->data.net, flags); - break; - case VIR_DOMAIN_DEVICE_INPUT: - rc = virDomainInputDefFormat(&buf, src->data.input, flags); - break; - case VIR_DOMAIN_DEVICE_SOUND: - rc = virDomainSoundDefFormat(&buf, src->data.sound, flags); - break; - case VIR_DOMAIN_DEVICE_VIDEO: - rc = virDomainVideoDefFormat(&buf, src->data.video, flags); - break; - case VIR_DOMAIN_DEVICE_HOSTDEV: - rc = virDomainHostdevDefFormat(&buf, src->data.hostdev, flags); - break; - case VIR_DOMAIN_DEVICE_WATCHDOG: - rc = virDomainWatchdogDefFormat(&buf, src->data.watchdog, flags); - break; - case VIR_DOMAIN_DEVICE_CONTROLLER: - rc = virDomainControllerDefFormat(&buf, src->data.controller, flags); - break; - case VIR_DOMAIN_DEVICE_GRAPHICS: - rc = virDomainGraphicsDefFormat(&buf, src->data.graphics, flags); - break; - case VIR_DOMAIN_DEVICE_HUB: - rc = virDomainHubDefFormat(&buf, src->data.hub, flags); - break; - case VIR_DOMAIN_DEVICE_REDIRDEV: - rc = virDomainRedirdevDefFormat(&buf, src->data.redirdev, flags); - break; - case VIR_DOMAIN_DEVICE_RNG: - rc = virDomainRNGDefFormat(&buf, src->data.rng, flags); - break; - case VIR_DOMAIN_DEVICE_CHR: - rc = virDomainChrDefFormat(&buf, src->data.chr, flags); - break; - case VIR_DOMAIN_DEVICE_NONE: - case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_MEMBALLOON: - case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Copying definition of '%d' type " - "is not implemented yet."), - src->type); - goto cleanup; - } + if (!(xmlStr = virDomainDeviceDefFormat(src, flags))) + return ret; - if (rc < 0) - goto cleanup; - - xmlStr = virBufferContentAndReset(&buf); ret = virDomainDeviceDefParse(xmlStr, def, caps, xmlopt, flags); -cleanup: VIR_FREE(xmlStr); return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9414ebf..a65ee3e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2310,6 +2310,8 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +char * virDomainDeviceDefFormat(virDomainDeviceDefPtr def, + unsigned int flags); virDomainDefPtr virDomainDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 50e2f48..34c1b17 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -173,6 +173,7 @@ virDomainDeleteConfig; virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; virDomainDeviceDefCopy; +virDomainDeviceDefFormat; virDomainDeviceDefFree; virDomainDeviceDefParse; virDomainDeviceFindControllerModel; -- 1.8.1.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 5 ++++ python/generator.py | 1 + python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 30 ++++++++++++++++++++++++ src/driver.h | 7 ++++++ src/libvirt.c | 51 +++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 7 files changed, 106 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a47e33c..2aa63a3 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -5055,6 +5055,11 @@ int virDomainFSTrim(virDomainPtr dom, unsigned long long minimum, unsigned int flags); +int virDomainNormalizeXML(virDomainPtr dom, + const char *xmlIn, + char **xmlOut, + unsigned int flags); + /** * virSchedParameterType: * diff --git a/python/generator.py b/python/generator.py index a91dde8..085f39d 100755 --- a/python/generator.py +++ b/python/generator.py @@ -460,6 +460,7 @@ skip_impl = ( 'virNodeGetCPUMap', 'virDomainMigrate3', 'virDomainMigrateToURI3', + 'virDomainNormalizeXML', ) lxc_skip_impl = ( diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 9a88215..55554d6 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -602,5 +602,12 @@ <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> <arg name='flags' type='int' info='unused, pass 0'/> </function> + <function name='virDomainNormalizeXML' file='python'> + <info>Normalize XML snippets</info> + <return type='char *' info='Normalized XML'/> + <arg name='domain' type='virDomainPtr' info='a domain object'/> + <arg name='xml' type='const char *' info='XML to normalize'/> + <arg name='flags' type='unsigned int' info='an OR'ed set of virDomainXMLFlags'/> + </function> </symbols> </api> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index d16b9a2..1ad06da 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -7162,6 +7162,35 @@ cleanup: } +static PyObject * +libvirt_virDomainNormalizeXML(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { + PyObject *py_retval = NULL; + virDomainPtr domain; + PyObject *pyobj_domain; + int c_retval; + char *xmlIn, *xmlOut; + unsigned int flags; + + if (!PyArg_ParseTuple(args, (char *)"Ozi:virDomainNormalizeXML", + &pyobj_domain, &xmlIn, &flags)) + return NULL; + + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainNormalizeXML(domain, xmlIn, &xmlOut, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval < 0) + goto cleanup; + + py_retval = libvirt_charPtrWrap(xmlOut); + +cleanup: + return py_retval; +} + + /************************************************************************ * * * The registration stuff * @@ -7289,6 +7318,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virNodeGetCPUMap", libvirt_virNodeGetCPUMap, METH_VARARGS, NULL}, {(char *) "virDomainCreateXMLWithFiles", libvirt_virDomainCreateXMLWithFiles, METH_VARARGS, NULL}, {(char *) "virDomainCreateWithFiles", libvirt_virDomainCreateWithFiles, METH_VARARGS, NULL}, + {(char *) "virDomainNormalizeXML", libvirt_virDomainNormalizeXML, METH_VARARGS, NULL}, {NULL, NULL, 0, NULL} }; diff --git a/src/driver.h b/src/driver.h index be64333..dcbee72 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1122,6 +1122,12 @@ typedef int unsigned int flags, int cancelled); +typedef int +(*virDrvDomainNormalizeXML)(virDomainPtr domain, + const char *xmlIn, + char **xmlOut, + unsigned int flas); + typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr; @@ -1332,6 +1338,7 @@ struct _virDriver { virDrvDomainMigratePerform3Params domainMigratePerform3Params; virDrvDomainMigrateFinish3Params domainMigrateFinish3Params; virDrvDomainMigrateConfirm3Params domainMigrateConfirm3Params; + virDrvDomainNormalizeXML domainNormalizeXML; }; diff --git a/src/libvirt.c b/src/libvirt.c index 3f65f12..e5eb231 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21967,3 +21967,54 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virDomainNormalizeXML: + * @domain: a domain object + * @xmlIn: XML to normalize + * @xmlOut: the result of normalization + * @flags: bitwise-OR of subset of virDomainXMLFlags + * + * Intruct libvirt to take @xmlIn, parse it and format again. This has effect + * that all returned values are formated as they would be a part of @dom. For + * example, if among passed @xmlIn and address='0x00008' occurs, it is + * formatted as address='0x08'. The result of normalization is returned in @xmlOut + * (automatically allocated array which is supposed to be freed by caller once + * no longer needed). However, if only validation of @xmlIn is desired, pass + * NULL as @xmlOut. + * + * Returns 0 on success -1 otherwise. + */ +int +virDomainNormalizeXML(virDomainPtr domain, + const char *xmlIn, + char **xmlOut, + unsigned int flags) +{ + int ret = -1; + VIR_DOMAIN_DEBUG(domain, "xmlIn=%s, xmlOut=%p, flags=%x", + NULLSTR(xmlIn), xmlOut, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + goto error; + } + + virCheckNonNullArgGoto(xmlIn, error); + + if (domain->conn->driver->domainNormalizeXML) { + ret = domain->conn->driver->domainNormalizeXML(domain, xmlIn, + xmlOut, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(domain->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index bbdf78a..7246266 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -634,4 +634,9 @@ LIBVIRT_1.1.1 { virDomainSetMemoryStatsPeriod; } LIBVIRT_1.1.0; +LIBVIRT_1.1.3 { + global: + virDomainNormalizeXML; +} LIBVIRT_1.1.1; + # .... define new API here using predicted next version number .... -- 1.8.1.5
On 09/17/2013 08:46 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 5 ++++ python/generator.py | 1 + python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 30 ++++++++++++++++++++++++ src/driver.h | 7 ++++++ src/libvirt.c | 51 +++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 7 files changed, 106 insertions(+)
+ +/** + * virDomainNormalizeXML: + * @domain: a domain object + * @xmlIn: XML to normalize + * @xmlOut: the result of normalization + * @flags: bitwise-OR of subset of virDomainXMLFlags + * + * Intruct libvirt to take @xmlIn, parse it and format again. This has effect
s/Intruct/Instruct/
+ * that all returned values are formated as they would be a part of @dom. For
s/formated/formatted/
+ * example, if among passed @xmlIn and address='0x00008' occurs, it is + * formatted as address='0x08'. The result of normalization is returned in @xmlOut + * (automatically allocated array which is supposed to be freed by caller once + * no longer needed). However, if only validation of @xmlIn is desired, pass + * NULL as @xmlOut. + * + * Returns 0 on success -1 otherwise. + */ +int +virDomainNormalizeXML(virDomainPtr domain, + const char *xmlIn, + char **xmlOut, + unsigned int flags)
When the topic came up in the past, the question was whether we can ALSO use this API to do systematic RelaxNG validation of the input xml (where xmlOut can be NULL if only validation is required, or a non-NULL pointer to also get the normalized result on success; or an error if validation fails). Of course, RelaxNG validation should be specified via flags (and not done all the time). Meanwhile, I'm not sure this is quite the right interface. When this topic came up last time, I had mentioned the idea of having it be virConnectNormalizeXML(virConnectPtr conn, int type, const char *xmlIn, char **xmlOut, unsigned int flags), where 'type' is an enum that says WHAT type of XML is being normalized (<domain>, a <disk> sub-element of domain, <network>, <domainsnapshot>, <nwfilter>, etc). In particular, since device hot-plug uses a subset of domain, it might be nice to support the normalization of a disk hotplug snippet without having to plug it into a larger <domain> XML - possible if we have an enum that tells the API which (subset of) schema we are targetting. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 18.09.2013 17:50, Eric Blake wrote:
On 09/17/2013 08:46 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 5 ++++ python/generator.py | 1 + python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 30 ++++++++++++++++++++++++ src/driver.h | 7 ++++++ src/libvirt.c | 51 +++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 7 files changed, 106 insertions(+)
+ +/** + * virDomainNormalizeXML: + * @domain: a domain object + * @xmlIn: XML to normalize + * @xmlOut: the result of normalization + * @flags: bitwise-OR of subset of virDomainXMLFlags + * + * Intruct libvirt to take @xmlIn, parse it and format again. This has effect
s/Intruct/Instruct/
+ * that all returned values are formated as they would be a part of @dom. For
s/formated/formatted/
+ * example, if among passed @xmlIn and address='0x00008' occurs, it is + * formatted as address='0x08'. The result of normalization is returned in @xmlOut + * (automatically allocated array which is supposed to be freed by caller once + * no longer needed). However, if only validation of @xmlIn is desired, pass + * NULL as @xmlOut. + * + * Returns 0 on success -1 otherwise. + */ +int +virDomainNormalizeXML(virDomainPtr domain, + const char *xmlIn, + char **xmlOut, + unsigned int flags)
When the topic came up in the past, the question was whether we can ALSO use this API to do systematic RelaxNG validation of the input xml (where xmlOut can be NULL if only validation is required, or a non-NULL pointer to also get the normalized result on success; or an error if validation fails). Of course, RelaxNG validation should be specified via flags (and not done all the time).
Meanwhile, I'm not sure this is quite the right interface. When this topic came up last time, I had mentioned the idea of having it be virConnectNormalizeXML(virConnectPtr conn, int type, const char *xmlIn, char **xmlOut, unsigned int flags), where 'type' is an enum that says WHAT type of XML is being normalized (<domain>, a <disk> sub-element of domain, <network>, <domainsnapshot>, <nwfilter>, etc). In particular, since device hot-plug uses a subset of domain, it might be nice to support the normalization of a disk hotplug snippet without having to plug it into a larger <domain> XML - possible if we have an enum that tells the API which (subset of) schema we are targetting.
Right, however, some of our device parsing functions require virDomainDefPtr (or some parts of it), for instance virDomainDiskDefParseXML(). To get rid of that would mean much more changes. Speaking of which, I think the better approach would be to split parse and check of XML into two separate steps. Or in fact three steps: parse, fill in defaults, sanity check. Michal
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/remote.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 40 +++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 +++++++++++++++++- 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..1acbbfb 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5304,3 +5304,53 @@ error: } return -1; } + +static int +remoteDispatchDomainNormalizeXML(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_normalize_xml_args *args, + remote_domain_normalize_xml_ret *ret) +{ + int rv = -1; + int norm_ret; + virDomainPtr dom = NULL; + char *xmlOut = NULL; + char **xmlOut_p = NULL; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + norm_ret = virDomainNormalizeXML(dom, + args->xmlIn, + args->need_xmlOut ? &xmlOut : NULL, + args->flags); + if (norm_ret < 0) + goto cleanup; + + if (args->need_xmlOut) { + if (VIR_ALLOC(xmlOut_p) < 0) + goto cleanup; + *xmlOut_p = xmlOut; + xmlOut = NULL; + } + + ret->ret = norm_ret; + ret->xmlOut = xmlOut_p; + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +} diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62e77a5..9128699 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6605,6 +6605,45 @@ remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) virDomainEventStateQueue(priv->domainEventState, event); } +static int +remoteDomainNormalizeXML(virDomainPtr dom, + const char *xmlIn, + char **xmlOut, + unsigned int flags) +{ + int rv = -1; + struct private_data *priv = dom->conn->privateData; + remote_domain_normalize_xml_args args; + remote_domain_normalize_xml_ret ret; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, dom); + args.xmlIn = (char *) xmlIn; + args.need_xmlOut = !!xmlOut; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_NORMALIZE_XML, + (xdrproc_t) xdr_remote_domain_normalize_xml_args, (char *) &args, + (xdrproc_t) xdr_remote_domain_normalize_xml_ret, (char *) &ret) == -1) + goto done; + + rv = ret.ret; + + if (rv == 0 && xmlOut) { + *xmlOut = *ret.xmlOut; + *ret.xmlOut = NULL; + } + +done: + xdr_free((xdrproc_t) xdr_remote_domain_normalize_xml_ret, (char *) &ret); + remoteDriverUnlock(priv); + return rv; +} + + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -6933,6 +6972,7 @@ static virDriver remote_driver = { .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */ + .domainNormalizeXML = remoteDomainNormalizeXML, /* 1.1.3 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 85ad9ba..d5539eb 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2835,6 +2835,18 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; }; +struct remote_domain_normalize_xml_args { + remote_nonnull_domain dom; + remote_nonnull_string xmlIn; + int need_xmlOut; + unsigned int flags; +}; + +struct remote_domain_normalize_xml_ret { + remote_string xmlOut; + int ret; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -4998,5 +5010,11 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311 + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, + + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_NORMALIZE_XML = 312 }; -- 1.8.1.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 8 +++++ 2 files changed, 102 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 49cd154..5075d0b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9467,6 +9467,94 @@ cleanup: /* + * "normalize" command + */ +static const vshCmdInfo info_normalize[] = { + {.name = "help", + .data = N_("reformat XML from a file") + }, + {.name = "desc", + .data = N_("Parse XML from a file and format it back")}, + {.name = NULL} +}; + +static const vshCmdOptDef opts_normalize[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "file", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("XML file") + }, + {.name = "inactive", + .type = VSH_OT_BOOL, + .help = N_("show inactive defined XML") + }, + {.name = "security-info", + .type = VSH_OT_BOOL, + .help = N_("include security sensitive information in XML dump") + }, + {.name = "update-cpu", + .type = VSH_OT_BOOL, + .help = N_("update guest CPU according to host CPU") + }, + {.name = "migratable", + .type = VSH_OT_BOOL, + .help = N_("provide XML suitable for migrations") + }, + {.name = NULL} +}; +static bool +cmdNormalize(vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + virDomainPtr dom; + const char *from; + char *buffer = NULL; + char *bufferOut = NULL; + unsigned int flags = 0; + bool inactive = vshCommandOptBool(cmd, "inactive"); + bool secure = vshCommandOptBool(cmd, "security-info"); + bool update = vshCommandOptBool(cmd, "update-cpu"); + bool migratable = vshCommandOptBool(cmd, "migratable"); + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return ret; + + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) + goto cleanup; + + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { + vshReportError(ctl); + goto cleanup; + } + + if (inactive) + flags |= VIR_DOMAIN_XML_INACTIVE; + if (secure) + flags |= VIR_DOMAIN_XML_SECURE; + if (update) + flags |= VIR_DOMAIN_XML_UPDATE_CPU; + if (migratable) + flags |= VIR_DOMAIN_XML_MIGRATABLE; + + if (virDomainNormalizeXML(dom, buffer, &bufferOut, flags) < 0) + goto cleanup; + + vshPrint(ctl, "%s", bufferOut); + ret = true; + +cleanup: + virDomainFree(dom); + VIR_FREE(buffer); + VIR_FREE(bufferOut); + return ret; +} + +/* * "detach-device" command */ static const vshCmdInfo info_detach_device[] = { @@ -10743,6 +10831,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_migrate_getspeed, .flags = 0 }, + {.name = "normalize", + .handler = cmdNormalize, + .opts = opts_normalize, + .info = info_normalize, + .flags = 0 + }, {.name = "numatune", .handler = cmdNumatune, .opts = opts_numatune, diff --git a/tools/virsh.pod b/tools/virsh.pod index 0ae5178..532b081 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1208,6 +1208,14 @@ migrated to another host. Get the maximum migration bandwidth (in MiB/s) for a domain. +=item B<normalize> I<domain> I<file> [I<--inactive>] [I<--security-info>] +[I<--update-cpu>] [I<--migratable>] + +Parse XML snippet stored in I<file> as it would be a part of I<domain> and +format it back. This has advantage of all values are formatted uniformly. For +instance, if I<file> contains address='0x0008' the output will reformat this as +address='0x08'. To control produced XML use the same options as B<dumpxml>. + =item B<numatune> I<domain> [I<--mode> B<mode>] [I<--nodeset> B<nodeset>] [[I<--config>] [I<--live>] | [I<--current>]] -- 1.8.1.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0763f9b..1f35c06 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15653,6 +15653,50 @@ cleanup: return ret; } +static int +qemuDomainNormalizeXML(virDomainPtr dom, + const char *xmlIn, + char **xmlOut, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + virDomainDeviceDefPtr dev_def = NULL; + virCapsPtr caps = NULL; + + /* Flags checked by virDomainDefFormat */ + + if (!(vm = qemuDomObjFromDomain(dom))) + return ret; + + if (virDomainNormalizeXMLEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) + flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS; + + if (!(dev_def = virDomainDeviceDefParse(xmlIn, vm->def, caps, + driver->xmlopt, + flags))) + goto cleanup; + + if (xmlOut && + !(*xmlOut = virDomainDeviceDefFormat(dev_def, flags))) + goto cleanup; + + ret = 0; + +cleanup: + virDomainDeviceDefFree(dev_def); + virObjectUnref(caps); + if (vm) + virObjectUnlock(vm); + return ret; +} static int qemuNodeGetInfo(virConnectPtr conn, @@ -15954,6 +15998,7 @@ static virDriver qemuDriver = { .domainMigratePerform3Params = qemuDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = qemuDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = qemuDomainMigrateConfirm3Params, /* 1.1.0 */ + .domainNormalizeXML = qemuDomainNormalizeXML, /* 1.1.3 */ }; -- 1.8.1.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 33 ++++++++++++++++++++------------- src/conf/domain_conf.h | 4 +++- src/openvz/openvz_conf.c | 1 + src/openvz/openvz_driver.c | 10 +++++++--- src/qemu/qemu_command.c | 7 +++---- src/vbox/vbox_tmpl.c | 1 + src/vmx/vmx.c | 2 ++ src/xenapi/xenapi_driver.c | 1 + src/xenxs/xen_sxpr.c | 1 + src/xenxs/xen_xm.c | 1 + tests/openvzutilstest.c | 1 - 11 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7c92e51..f2a96e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2802,7 +2802,8 @@ virDomainDefPostParseInternal(virDomainDefPtr def, static int virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, virDomainDefPtr def, - virCapsPtr caps ATTRIBUTE_UNUSED) + virCapsPtr caps ATTRIBUTE_UNUSED, + virDomainXMLOptionPtr xmlopt) { if (dev->type == VIR_DOMAIN_DEVICE_CHR) { virDomainChrDefPtr chr = dev->data.chr; @@ -2845,6 +2846,13 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, } } + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virDomainNetDefPtr net = dev->data.net; + + if (!net->macUsable) + virDomainNetGenerateMAC(xmlopt, net); + } + return 0; } @@ -2864,7 +2872,7 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return ret; } - if ((ret = virDomainDeviceDefPostParseInternal(dev, def, caps)) < 0) + if ((ret = virDomainDeviceDefPostParseInternal(dev, def, caps, xmlopt)) < 0) return ret; return 0; @@ -5892,9 +5900,10 @@ cleanup: void virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, - virMacAddrPtr mac) + virDomainNetDefPtr net) { - virMacAddrGenerate(xmlopt->config.macPrefix, mac); + virMacAddrGenerate(xmlopt->config.macPrefix, &net->mac); + net->macUsable = true; } @@ -6218,8 +6227,7 @@ error: * @return 0 on success, -1 on failure */ static virDomainNetDefPtr -virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, - xmlNodePtr node, +virDomainNetDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, unsigned int flags) @@ -6402,8 +6410,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, (const char *)macaddr); goto error; } - } else { - virDomainNetGenerateMAC(xmlopt, &def->mac); + def->macUsable = true; } if (devaddr) { @@ -9522,7 +9529,7 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_NET: - if (!(dev->data.net = virDomainNetDefParseXML(xmlopt, node, ctxt, + if (!(dev->data.net = virDomainNetDefParseXML(node, ctxt, NULL, flags))) goto error; break; @@ -11851,8 +11858,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->nets, n) < 0) goto error; for (i = 0; i < n; i++) { - virDomainNetDefPtr net = virDomainNetDefParseXML(xmlopt, - nodes[i], + virDomainNetDefPtr net = virDomainNetDefParseXML(nodes[i], ctxt, bootHash, flags); @@ -14945,8 +14951,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 6); - virBufferAsprintf(buf, "<mac address='%s'/>\n", - virMacAddrFormat(&def->mac, macstr)); + if (def->macUsable) + virBufferAsprintf(buf, "<mac address='%s'/>\n", + virMacAddrFormat(&def->mac, macstr)); switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a65ee3e..0ac3478 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -974,6 +974,7 @@ struct _virDomainActualNetDef { struct _virDomainNetDef { enum virDomainNetType type; virMacAddr mac; + bool macUsable; /* @mac has been parsed and is set */ char *model; union { struct { @@ -2166,7 +2167,8 @@ virDomainXMLOptionPtr virDomainXMLOptionNew(virDomainDefParserConfigPtr config, virDomainXMLPrivateDataCallbacksPtr priv, virDomainXMLNamespacePtr xmlns); -void virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, virMacAddrPtr mac); +void virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, + virDomainNetDefPtr net); virDomainXMLNamespacePtr virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 0dbaa4a..a95da19 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -321,6 +321,7 @@ openvzReadNetworkConf(virDomainDefPtr def, _("Wrong MAC address")); goto error; } + net->macUsable = true; } p = ++next; } while (p < token + strlen(token)); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d268647..993e037 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -809,7 +809,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, { int rc = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; - virMacAddr host_mac; + virDomainNetDefPtr hostDev = NULL; char host_macaddr[VIR_MAC_STRING_BUFLEN]; struct openvz_driver *driver = conn->privateData; virCommandPtr cmd = NULL; @@ -827,11 +827,14 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) return 0; + if (VIR_ALLOC(hostDev) < 0) + return rc; + cmd = virCommandNewArgList(VZCTL, "--quiet", "set", vpsid, NULL); virMacAddrFormat(&net->mac, macaddr); - virDomainNetGenerateMAC(driver->xmlopt, &host_mac); - virMacAddrFormat(&host_mac, host_macaddr); + virDomainNetGenerateMAC(driver->xmlopt, hostDev); + virMacAddrFormat(&hostDev->mac, host_macaddr); if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && @@ -899,6 +902,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid, cleanup: virCommandFree(cmd); VIR_FREE(guest_ifname); + virDomainNetDefFree(hostDev); return rc; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4628dac..d83d015 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10244,7 +10244,6 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, const char *nic; int wantvlan = 0; const char *tmp; - bool genmac = true; size_t i; tmp = strchr(val, ','); @@ -10333,7 +10332,6 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, for (i = 0; i < nkeywords; i++) { if (STREQ(keywords[i], "macaddr")) { - genmac = false; if (virMacAddrParse(values[i], &def->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse mac address '%s'"), @@ -10342,6 +10340,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, def = NULL; goto cleanup; } + def->macUsable = true; } else if (STREQ(keywords[i], "model")) { def->model = values[i]; values[i] = NULL; @@ -10363,8 +10362,8 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, } } - if (genmac) - virDomainNetGenerateMAC(xmlopt, &def->mac); + if (!def->macUsable) + virDomainNetGenerateMAC(xmlopt, def); cleanup: for (i = 0; i < nkeywords; i++) { diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 5e5ea85..66e6cbc 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3031,6 +3031,7 @@ sharedFoldersCleanup: /* XXX some real error handling here some day ... */ if (virMacAddrParse(macaddr, &def->nets[netAdpIncCnt]->mac) < 0) {} + def->nets[netAdpIncCnt]->macUsable = true; netAdpIncCnt++; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 40416a0..8410473 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2510,6 +2510,7 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def) generatedAddress); goto cleanup; } + (*def)->macUsable = true; } } else if (STRCASEEQ(addressType, "static")) { if (address != NULL) { @@ -2519,6 +2520,7 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def) "found '%s'"), address_name, address); goto cleanup; } + (*def)->macUsable = true; } } else { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index bca19af..d13e712 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1519,6 +1519,7 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) &defPtr->nets[i]->mac) < 0) xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, _("Unable to parse given mac address")); + defPtr->nets[i]->macUsable = true; xen_vif_record_free(vif_rec); } } diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 6209c68..9d1d374 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -589,6 +589,7 @@ xenParseSxprNets(virDomainDefPtr def, _("malformed mac address '%s'"), tmp); goto cleanup; } + net->macUsable = true; } if (VIR_STRDUP(net->model, model) < 0) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 1ffea84..79bd952 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -743,6 +743,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, _("malformed mac address '%s'"), mac); goto cleanup; } + net->macUsable = true; } if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || diff --git a/tests/openvzutilstest.c b/tests/openvzutilstest.c index ee68c06..f418e50 100644 --- a/tests/openvzutilstest.c +++ b/tests/openvzutilstest.c @@ -94,7 +94,6 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED) " <on_crash>destroy</on_crash>\n" " <devices>\n" " <interface type='ethernet'>\n" - " <mac address='00:00:00:00:00:00'/>\n" " <ip address='194.44.18.88'/>\n" " </interface>\n" " <interface type='bridge'>\n" -- 1.8.1.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 1 + src/libxl/libxl_driver.c | 12 ++++++------ src/lxc/lxc_driver.c | 6 +++--- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 8 ++++---- src/uml/uml_driver.c | 4 ++-- src/vbox/vbox_tmpl.c | 4 ++-- src/xen/xend_internal.c | 6 +++--- src/xen/xm_internal.c | 8 ++++++-- tests/qemuhotplugtest.c | 3 ++- 12 files changed, 35 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f2a96e9..9a5cfe8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9481,6 +9481,7 @@ virDomainDeviceDefParse(const char *xmlStr, virDomainDefPtr def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + bool doPostParse, unsigned int flags) { xmlDocPtr xml; @@ -9604,7 +9605,8 @@ virDomainDeviceDefParse(const char *xmlStr, } /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParse(dev, def, caps, xmlopt) < 0) + if (doPostParse && + virDomainDeviceDefPostParse(dev, def, caps, xmlopt) < 0) goto error; cleanup: @@ -18233,7 +18235,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, if (!(xmlStr = virDomainDeviceDefFormat(src, flags))) return ret; - ret = virDomainDeviceDefParse(xmlStr, def, caps, xmlopt, flags); + ret = virDomainDeviceDefParse(xmlStr, def, caps, xmlopt, true, flags); VIR_FREE(xmlStr); return ret; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0ac3478..1d58bfa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2311,6 +2311,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, virDomainDefPtr def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + bool doPostParse, unsigned int flags); char * virDomainDeviceDefFormat(virDomainDeviceDefPtr def, unsigned int flags); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e2a6d44..326ade1 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3172,7 +3172,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; /* Make a copy for updated domain. */ @@ -3191,7 +3191,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; if ((ret = libxlDomainAttachDeviceLive(priv, vm, dev)) < 0) @@ -3276,7 +3276,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; /* Make a copy for updated domain. */ @@ -3295,7 +3295,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; if ((ret = libxlDomainDetachDeviceLive(priv, vm, dev)) < 0) @@ -3380,7 +3380,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; /* Make a copy for updated domain. */ @@ -3399,7 +3399,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; if ((ret = libxlDomainUpdateDeviceLive(priv, vm, dev)) < 0) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 4cf0b50..7d672b3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4053,7 +4053,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -4179,7 +4179,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -4289,7 +4289,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 993e037..a92c761 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -2093,7 +2093,7 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, goto cleanup; dev = virDomainDeviceDefParse(xml, vmdef, driver->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (!dev) goto cleanup; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 32165ed..fa50c2d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1674,7 +1674,7 @@ phypDomainAttachDevice(virDomainPtr domain, const char *xml) goto cleanup; dev = virDomainDeviceDefParse(xml, def, phyp_driver->caps, NULL, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (!dev) { goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1f35c06..dd8d6d2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6894,7 +6894,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - parse_flags); + true, parse_flags); if (dev == NULL) goto endjob; @@ -7037,7 +7037,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto endjob; @@ -7176,7 +7176,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - parse_flags); + true, parse_flags); if (dev == NULL) goto endjob; @@ -15681,7 +15681,7 @@ qemuDomainNormalizeXML(virDomainPtr dom, if (!(dev_def = virDomainDeviceDefParse(xmlIn, vm->def, caps, driver->xmlopt, - flags))) + false, flags))) goto cleanup; if (xmlOut && diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 9ca352f..e5b0fb1 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2198,7 +2198,7 @@ static int umlDomainAttachDevice(virDomainPtr dom, const char *xml) } dev = virDomainDeviceDefParse(xml, vm->def, driver->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -2318,7 +2318,7 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml) { } dev = virDomainDeviceDefParse(xml, vm->def, driver->caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 66e6cbc..d56512f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5381,7 +5381,7 @@ static int vboxDomainAttachDeviceImpl(virDomainPtr dom, goto cleanup; dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -5610,7 +5610,7 @@ static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml) { goto cleanup; dev = virDomainDeviceDefParse(xml, def, data->caps, data->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + true, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index f698c8d..5ebbb4a 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2246,7 +2246,7 @@ xenDaemonAttachDeviceFlags(virConnectPtr conn, goto cleanup; if (!(dev = virDomainDeviceDefParse(xml, def, priv->caps, priv->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; @@ -2394,7 +2394,7 @@ xenDaemonUpdateDeviceFlags(virConnectPtr conn, goto cleanup; if (!(dev = virDomainDeviceDefParse(xml, def, priv->caps, priv->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; @@ -2496,7 +2496,7 @@ xenDaemonDetachDeviceFlags(virConnectPtr conn, goto cleanup; if (!(dev = virDomainDeviceDefParse(xml, def, priv->caps, priv->xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; if (virDomainXMLDevID(conn, minidef, dev, class, ref, sizeof(ref))) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index d20dd91..3e890c5 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1245,9 +1245,11 @@ xenXMDomainAttachDeviceFlags(virConnectPtr conn, goto cleanup; def = entry->def; - if (!(dev = virDomainDeviceDefParse(xml, entry->def, + if (!(dev = virDomainDeviceDefParse(xml, + entry->def, priv->caps, priv->xmlopt, + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; @@ -1334,9 +1336,11 @@ xenXMDomainDetachDeviceFlags(virConnectPtr conn, goto cleanup; def = entry->def; - if (!(dev = virDomainDeviceDefParse(xml, entry->def, + if (!(dev = virDomainDeviceDefParse(xml, + entry->def, priv->caps, priv->xmlopt, + true, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 89480af..d7a49c7 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -245,7 +245,8 @@ testQemuHotplug(const void *data) } if (!(dev = virDomainDeviceDefParse(device_xml, vm->def, - caps, driver.xmlopt, 0))) + caps, driver.xmlopt, + true, 0))) goto cleanup; /* Now is the best time to feed the spoofed monitor with predefined -- 1.8.1.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 151 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5075d0b..6a60a40 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9357,7 +9357,6 @@ error: * @n2 second node * returns true in case n1 covers n2, false otherwise. */ -ATTRIBUTE_UNUSED static bool vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) { @@ -9465,6 +9464,139 @@ cleanup: return ret; } +/** + * vshCompleteXMLFromDomain: + * @ctl vshControl for error messages printing + * @dom domain + * @oldXML device XML before + * @newXML and after completion + * + * For given domain and (probably incomplete) device XML specification try to + * find such device in domain and complete missing parts. This is however + * possible only when given device XML is sufficiently precise so it addresses + * only one device. + * + * Returns -2 when no such device exists in domain, -3 when given XML selects many + * (is too ambiguous), 0 in case of success. Otherwise returns -1. @newXML + * is touched only in case of success. + */ +static int +vshCompleteXMLFromDomain(vshControl *ctl, virDomainPtr dom, + const char *oldXML, char **newXML) +{ + int funcRet = -1; + char *domXML = NULL, *normalizedXML = NULL; + xmlDocPtr domDoc = NULL, devDoc = NULL; + xmlNodePtr node = NULL; + xmlXPathContextPtr domCtxt = NULL, devCtxt = NULL; + xmlNodePtr *devices = NULL; + xmlSaveCtxtPtr sctxt = NULL; + int devices_size; + char *xpath = NULL; + xmlBufferPtr buf = NULL; + size_t i = 0; + ssize_t indx = -1; + + if (!(domXML = virDomainGetXMLDesc(dom, 0))) { + vshError(ctl, _("couldn't get XML description of domain %s"), + virDomainGetName(dom)); + goto cleanup; + } + + domDoc = virXMLParseStringCtxt(domXML, _("(domain_definition)"), &domCtxt); + if (!domDoc) { + vshError(ctl, _("Failed to parse domain definition xml")); + goto cleanup; + } + + /* Prior parsing oldXML try to normalize it */ + if (virDomainNormalizeXML(dom, oldXML, &normalizedXML, 0) >= 0) { + /* Yay, success! Deliberately not freeing oldXML here, as it's caller + * who is supposed to do so. */ + oldXML = normalizedXML; + } + + devDoc = virXMLParseStringCtxt(oldXML, _("(device_definition)"), &devCtxt); + if (!devDoc) { + vshError(ctl, _("Failed to parse device definition xml")); + goto cleanup; + } + + node = xmlDocGetRootElement(devDoc); + + buf = xmlBufferCreate(); + if (!buf) { + vshError(ctl, "%s", _("out of memory")); + goto cleanup; + } + + /* Get all possible devices */ + if (virAsprintf(&xpath, "/domain/devices/%s", node->name) < 0) + goto cleanup; + + devices_size = virXPathNodeSet(xpath, domCtxt, &devices); + + if (devices_size < 0) { + /* error */ + vshError(ctl, "%s", _("error when selecting nodes")); + goto cleanup; + } else if (devices_size == 0) { + /* no such device */ + funcRet = -2; + goto cleanup; + } + + /* and refine */ + for (i = 0; i < devices_size; i++) { + if (vshNodeIsSuperset(devices[i], node)) { + if (indx >= 0) { + funcRet = -3; /* ambiguous */ + goto cleanup; + } + indx = i; + } + } + + if (indx < 0) { + funcRet = -2; /* no such device */ + goto cleanup; + } + + vshDebug(ctl, VSH_ERR_DEBUG, "Found device at pos %zd\n", indx); + + if (newXML) { + sctxt = xmlSaveToBuffer(buf, NULL, 0); + if (!sctxt) { + vshError(ctl, "%s", _("failed to create document saving context")); + goto cleanup; + } + + xmlSaveTree(sctxt, devices[indx]); + xmlSaveClose(sctxt); + *newXML = (char *) xmlBufferContent(buf); + if (!*newXML) { + virReportOOMError(); + goto cleanup; + } + buf->content = NULL; + } + + vshDebug(ctl, VSH_ERR_DEBUG, "Old xml:\n%s\nNew xml:\n%s\n", oldXML, + newXML ? NULLSTR(*newXML) : "(null)"); + + funcRet = 0; + +cleanup: + xmlBufferFree(buf); + VIR_FREE(devices); + xmlXPathFreeContext(devCtxt); + xmlXPathFreeContext(domCtxt); + xmlFreeDoc(devDoc); + xmlFreeDoc(domDoc); + VIR_FREE(domXML); + VIR_FREE(xpath); + return funcRet; +} /* * "normalize" command @@ -9602,7 +9734,7 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; const char *from = NULL; - char *buffer = NULL; + char *buffer = NULL, *new_buffer = NULL; int ret; bool funcRet = false; bool current = vshCommandOptBool(cmd, "current"); @@ -9636,10 +9768,24 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + ret = vshCompleteXMLFromDomain(ctl, dom, buffer, &new_buffer); + if (ret < 0) { + if (ret == -2) { + vshError(ctl, _("no such device in %s"), virDomainGetName(dom)); + } else if (ret == -3) { + vshError(ctl, "%s", _("given XML selects too many devices. " + "Please, be more specific")); + } else { + /* vshCompleteXMLFromDomain() already printed error message, + * so nothing to do here. */ + } + goto cleanup; + } + if (flags != 0) - ret = virDomainDetachDeviceFlags(dom, buffer, flags); + ret = virDomainDetachDeviceFlags(dom, new_buffer, flags); else - ret = virDomainDetachDevice(dom, buffer); + ret = virDomainDetachDevice(dom, new_buffer); if (ret < 0) { vshError(ctl, _("Failed to detach device from %s"), from); @@ -9651,6 +9797,7 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_FREE(buffer); + VIR_FREE(new_buffer); virDomainFree(dom); return funcRet; } -- 1.8.1.5
On 09/17/2013 10:46 AM, Michal Privoznik wrote:
A while ago I've invented this vshCompleteXMLFromDomain() function to increase device-detach intelligence. Basically, it took user's XML and tried to find matching device in domain's XML. However, it was kind of buggy - finding the matching device uses string comparison. This works on text values. It doesn't on integer ones (who would expect that). So prior the lookup process, we need to normalize the integer values (and whole XML). However, this can't be done on the client side, since he has no knowledge which values are integer and which are not. Therefore we need a new API.
Michal Privoznik (8): domain_conf: Introduce virDomainDeviceDefFormat Introduce new virDomainNormalizeXML API remote_driver: Implement virDomainNormalizeXML virsh: Expose virDomainNormalizeXML qemu: Implement vimDomainNormalizeXML domain_conf: Move MAC generation to post parse callback virDomainDeviceDefParse: Make PostParse callback call optional virsh: Resurrect vshCompleteXMLFromDomain
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API? Why not allow something like virsh detach-device --device-number disk:2 Which will remove the second disk device? It's friendly-ish, easy to extend, and simple to implement. - Cole
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them: Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
Why not allow something like
virsh detach-device --device-number disk:2
Which will remove the second disk device? It's friendly-ish, easy to extend, and simple to implement.
That may be an orthogonal improvement to detach-device, but in no way diminishes the utility of this proposed API. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/18/2013 09:38 AM, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
In fact, I'd love to have 'virsh edit' and friends take advantage of normalization. Right now, if you run 'virsh edit' and type in something that libvirt doesn't recognize, it gets silently discarded; but if we add normalization into the mix and do a strcmp of the xml before the user's edits and after the normalization of the edits, we can more easily warn the user that not all their edits can be honored, and give them a chance to try again. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Sep 18, 2013 at 09:41:40AM -0600, Eric Blake wrote:
On 09/18/2013 09:38 AM, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
In fact, I'd love to have 'virsh edit' and friends take advantage of normalization. Right now, if you run 'virsh edit' and type in something that libvirt doesn't recognize, it gets silently discarded; but if we add normalization into the mix and do a strcmp of the xml before the user's edits and after the normalization of the edits, we can more easily warn the user that not all their edits can be honored, and give them a chance to try again.
That's a job for RNG schema validation IMHO. Round-tripping XML and doing a strcmp is really poor-mans schema validation. 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 09/18/2013 09:43 AM, Daniel P. Berrange wrote:
In fact, I'd love to have 'virsh edit' and friends take advantage of normalization. Right now, if you run 'virsh edit' and type in something that libvirt doesn't recognize, it gets silently discarded; but if we add normalization into the mix and do a strcmp of the xml before the user's edits and after the normalization of the edits, we can more easily warn the user that not all their edits can be honored, and give them a chance to try again.
That's a job for RNG schema validation IMHO. Round-tripping XML and doing a strcmp is really poor-mans schema validation.
Okay, but then see my comment on 3/8 that we should have one of the 'flags' values be supported for additionally requesting programmatic RNG validation as part of the normalization efforts. Seen in that light, 'virsh edit' should not do a strcmp of the round trip, but should instead use the 'RNG_VALIDATE' flag - and can then alert the user to any situation where the domain is invalid because of unknown elements. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Sep 18, 2013 at 09:38:23AM -0600, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
IMHO a better way to deal with that usage scenario is to add ability to validate XML via the RNG schema to the API. eg we should add a "validate" flag that can be passed to virDomainDefineXML / virDomainCreateXML that would enable RNG validation during parsing of those APis. And possibly even have a dedicated API to request validation of an XML doc in general (virt-xml-validate, but part of the API instead of local CLI tool) 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 09/18/2013 11:43 AM, Daniel P. Berrange wrote:
On Wed, Sep 18, 2013 at 09:38:23AM -0600, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
IMHO a better way to deal with that usage scenario is to add ability to validate XML via the RNG schema to the API.
eg we should add a "validate" flag that can be passed to virDomainDefineXML / virDomainCreateXML that would enable RNG validation during parsing of those APis. And possibly even have a dedicated API to request validation of an XML doc in general (virt-xml-validate, but part of the API instead of local CLI tool)
Yeah that could be useful. Though so would an API to just return the libvirt domain rng and let apps cache it, or parse it and use that as a poor man's XML capabilities and make virt-install --disk cache=? list all available cache modes without having to hardcode them. Granted that won't be accurate across hypervisors but its a start. - Cole
On Wed, Sep 18, 2013 at 11:55:39AM -0400, Cole Robinson wrote:
On 09/18/2013 11:43 AM, Daniel P. Berrange wrote:
On Wed, Sep 18, 2013 at 09:38:23AM -0600, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
IMHO a better way to deal with that usage scenario is to add ability to validate XML via the RNG schema to the API.
eg we should add a "validate" flag that can be passed to virDomainDefineXML / virDomainCreateXML that would enable RNG validation during parsing of those APis. And possibly even have a dedicated API to request validation of an XML doc in general (virt-xml-validate, but part of the API instead of local CLI tool)
Yeah that could be useful. Though so would an API to just return the libvirt domain rng and let apps cache it, or parse it and use that as a poor man's XML capabilities and make virt-install --disk cache=? list all available cache modes without having to hardcode them. Granted that won't be accurate across hypervisors but its a start.
I would not want apps to be in the business of parsing RNG docs to extract data. We make no guarantees about the structure of the RNG documents being stable at all. We can arbitrarily re-structure them at any time to suit our current needs. The only thing RNG should be used for is validating a document. This is the reason why I also think any tools which generate APIs based on RNG schemas are a bad idea. 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 09/18/2013 12:02 PM, Daniel P. Berrange wrote:
On Wed, Sep 18, 2013 at 11:55:39AM -0400, Cole Robinson wrote:
On 09/18/2013 11:43 AM, Daniel P. Berrange wrote:
On Wed, Sep 18, 2013 at 09:38:23AM -0600, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
IMHO a better way to deal with that usage scenario is to add ability to validate XML via the RNG schema to the API.
eg we should add a "validate" flag that can be passed to virDomainDefineXML / virDomainCreateXML that would enable RNG validation during parsing of those APis. And possibly even have a dedicated API to request validation of an XML doc in general (virt-xml-validate, but part of the API instead of local CLI tool)
Yeah that could be useful. Though so would an API to just return the libvirt domain rng and let apps cache it, or parse it and use that as a poor man's XML capabilities and make virt-install --disk cache=? list all available cache modes without having to hardcode them. Granted that won't be accurate across hypervisors but its a start.
I would not want apps to be in the business of parsing RNG docs to extract data. We make no guarantees about the structure of the RNG documents being stable at all. We can arbitrarily re-structure them at any time to suit our current needs. The only thing RNG should be used for is validating a document. This is the reason why I also think any tools which generate APIs based on RNG schemas are a bad idea.
I was going on the assumption that there's a standard way using libxml to say 'parse this rng' and 'give me listed values for /domain/devices/disk/driver@cache' but maybe that's not true. - Cole
On Wed, Sep 18, 2013 at 12:05:03PM -0400, Cole Robinson wrote:
On 09/18/2013 12:02 PM, Daniel P. Berrange wrote:
On Wed, Sep 18, 2013 at 11:55:39AM -0400, Cole Robinson wrote:
On 09/18/2013 11:43 AM, Daniel P. Berrange wrote:
On Wed, Sep 18, 2013 at 09:38:23AM -0600, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
IMHO a better way to deal with that usage scenario is to add ability to validate XML via the RNG schema to the API.
eg we should add a "validate" flag that can be passed to virDomainDefineXML / virDomainCreateXML that would enable RNG validation during parsing of those APis. And possibly even have a dedicated API to request validation of an XML doc in general (virt-xml-validate, but part of the API instead of local CLI tool)
Yeah that could be useful. Though so would an API to just return the libvirt domain rng and let apps cache it, or parse it and use that as a poor man's XML capabilities and make virt-install --disk cache=? list all available cache modes without having to hardcode them. Granted that won't be accurate across hypervisors but its a start.
I would not want apps to be in the business of parsing RNG docs to extract data. We make no guarantees about the structure of the RNG documents being stable at all. We can arbitrarily re-structure them at any time to suit our current needs. The only thing RNG should be used for is validating a document. This is the reason why I also think any tools which generate APIs based on RNG schemas are a bad idea.
I was going on the assumption that there's a standard way using libxml to say 'parse this rng' and 'give me listed values for /domain/devices/disk/driver@cache' but maybe that's not true.
That's not the way RNG works I'm afraid. It is rather more like a grammer used to define a programming language. A set of rules which you follow when parsing an input doc. You can't easily analyse such rules and say what is allowed by them, since they just designed to be executed/applied, not queried as an information source 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 09/18/2013 11:38 AM, Eric Blake wrote:
On 09/18/2013 09:32 AM, Cole Robinson wrote:
Seems overkill IMO. Do we really need to add an API to facilitate the crappy interface that is virsh detach-device? Will any other application ever want to use this API?
Yes, I can envision other use cases. In fact, virt-manager is one of them:
Oftentimes, we add features to XML, but you don't have an easy way to probe if the feature is supported. Rather than complicate the (already large) capabilities xml to call out yet more features, it is rather simple to write up XML that tries to use the feature, then run it through the normalizing API, then look at the result. If the feature is still present in the output, then libvirt understands the feature (and you can safely use it); if the feature got stripped as unrecognized, then you can issue a much nicer error message to the user stating that the libvirtd on the other end of the connection can't honor the user's request.
That sounds very painful: form XML, make API call, parse XML, examine every suspect parameter to see if it changed, report an error that libvirt can already give us. It also doesn't help us do something like 'list all disk cache modes this connection supports' or similar without doing multiple API calls, and would still require virt-manager to hardcode values just to probe for them. - Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Michal Privoznik