If the domain is running, the live def is used to parse the device XML, otherwise persistent def is used. --- src/qemu/qemu_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-)

+static char * +qemuDomainNormalizeDeviceXML(virDomainPtr dom, + const char *device_xml, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + virDomainDefPtr persistentDef = NULL; + virDomainDefPtr def = NULL; + virDomainDeviceDefPtr dev = NULL; + char *ret = NULL;

+ + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver);

+ + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + def = vm->def; + } else { + def = persistentDef; + }

On Mon, Jan 09, 2012 at 10:29:08PM +0800, Osier Yang wrote: > The initial purpose was to fix a regression for detaching device, > (introduced by commit ea7182c29). There was a patch posted to > resolve the problem: > > https://www.redhat.com/archives/libvir-list/2011-December/msg00818.html > > But as Eric suggested, it's not the ideal way to go, we never known > how many stuffs like <address> will be involved in future. So new API > to invoke the internal parsing functions might be a right way then. > > However, things are not that simple (an API without internal driver > support, invoking the parsing functions directly). As the domain conf > is a neccessary argument to parse the device XML. (e.g. for an Input

> device). Although we can bypass that by modification on > virDomainDeviceDefParse, it could be trap as we will parse the device > XML in another way which is different with the real parsing. So finally > I choosed to implement the driver support for the new API. There might

> be something I didn't take into consideration, (e.g. Do we need some > flags for the XML parsing and formating?). But it can demo the thought

> good enough. > > On the other hand, I'm wondering if it's deserved to introduce such > an API, comparing it's usage, is it two heavy if we need the internal > drivers support for such an API? > > Any thoughts and feedback is welcomed. I don't really understand what the use case is for this API, from the description above. Can you give an example of how, for example, virt-manager would use this API todo something ?

These 2 things feel like overloading the API with two different semantics. On the one hand I see a simple XML -> struct -> XML transformation, which is driver indepedant and does not need to get any HV state. I call this canonicalization

The other case I see XML -> struct -> munge struct info -> XML which makes changes based on the HV state. I call this XML enhancement. I'm not even convinced this is desirable. IMHO the way virsh detach device is going about this task is just plain wrong. It should be just requesting the full XML document, and then doing an XPath query on it, to pull out the <interface> element it needs. This removes the need to create any XML from scratch.

> * This function parses the incoming @xml, with root element > * according to @type, and returns the same XML normalized to the > * same form it would appear via the corresponding vir*GetXMLDesc > * function if it had described a real libvirt object. No > * machine state is checked or changed, so while the XML may be > * valid, subsequent use of the XML might still fail for other > * reasons such as an attempt to reuse a UUID. > *

This does look more useful, but as mentioned above, IMHO the normalization should be purely syntactic normalization, and not rely on any semantic state from the hypervisor drivers.

On 01/09/2012 07:37 AM, Daniel P. Berrange wrote: > On Mon, Jan 09, 2012 at 10:29:08PM +0800, Osier Yang wrote: >> The initial purpose was to fix a regression for detaching device, >> (introduced by commit ea7182c29). There was a patch posted to >> resolve the problem: >> >> https://www.redhat.com/archives/libvir-list/2011-December/msg00818.html >> >> But as Eric suggested, it's not the ideal way to go, we never known >> how many stuffs like<address> will be involved in future. So new API >> to invoke the internal parsing functions might be a right way then. >> >> However, things are not that simple (an API without internal driver >> support, invoking the parsing functions directly). As the domain conf >> is a neccessary argument to parse the device XML. (e.g. for an Input s/neccessary/necessary/ >> device). Although we can bypass that by modification on >> virDomainDeviceDefParse, it could be trap as we will parse the device >> XML in another way which is different with the real parsing. So finally >> I choosed to implement the driver support for the new API. There might s/choosed/chose/ >> be something I didn't take into consideration, (e.g. Do we need some >> flags for the XML parsing and formating?). But it can demo the thought s/formating/formatting/ >> good enough. >> >> On the other hand, I'm wondering if it's deserved to introduce such >> an API, comparing it's usage, is it two heavy if we need the internal >> drivers support for such an API? >> >> Any thoughts and feedback is welcomed. > > I don't really understand what the use case is for this API, from > the description above. Can you give an example of how, for example, > virt-manager would use this API todo something ? I see several potential uses: 1. XML feature probe. For example, libvirt 0.9.9 just added the XML feature of having a<seclabel> attached to a specific<disk>; older libvirt silently ignores that label. Right now, I have no way to know if I use<seclabel> if I will get a per-disk override; running virt-xml-validate on my machine doesn't help if the libvirt on the remote server is at a different libvirt version, and if my only connection to the remote machine is via libvirt connections rather than a direct ssh, then I can't run virt-xml-validate on the destination machine. But with this new API, I could: construct a candidate XML run it through the API compare the resulting XML with my candidate if<seclabel> is still intact, then the feature is supported; if it was stripped, then I am talking to an older libvirt that didn't know the feature 2. XML subset matching. Things like 'virsh detach-device' want to be permissive in what the user passes in, by taking minimal XML from the user and expanding it to the exact XML present in the domain. But since we allow interleaves, it's not easy to do the comparison from user XML to the order that the XML is generated by 'virsh dumpxml'. With the new API, virsh could be coded to run any candidate XML through the normalization, prior to doing subset comparisons against the real domain XML, and have the assurance that all known elements and attributes appear in the same order. 3. XML validation. It seems pretty easy to expand this API into taking a flag that says to compare the incoming XML against the RNG, and return NULL on failure to validate. This would make implementation of 'virsh edit' smarter, by being able to validate the user's edits prior to trying to apply them. 4. libvirt-gconfig seems like an obvious client for taking user input and normalizing it into the same form that libvirt would output. However, I think the API needs to be a bit more flexible than what Osier proposed. It needs to be an API on the virConnectPtr, not a virDomainPtr, since it is NOT tied to a specific domain, but to the parsing abilities of the connection in general. I also think that the API can handle multiple XML types in a single API. (Oh yuck - I just realized that VIR_DOMAIN_XML_INACTIVE==2, but VIR_INTERFACE_XML_INACTIVE==1, so we _can't_ have a common VIR_NORMALIZE_XML_INACTIVE). I envision something more like: typedef enum { /* All top-level elements where we have a getXMLDesc method: */ VIR_NORMALIZE_XML_DOMAIN = 100, /* top-level is<domain> */ VIR_NORMALIZE_XML_SNAPSHOT = 200, /* top-level is<domainsnapshot> */ VIR_NORMALIZE_XML_NETWORK = 300, /* top-level is<network> */ ... /* Additionally, sub-level elements that are used in various APIs: */ VIR_NORMALIZE_XML_DOMAIN_DEVICE = 101, /* any element that fits under /domain/devices, such as<disk> or<interface> */ ... } virConnectNormalizeXMLTypes; typedef enum { /* Reserve bits 0, 1, 2 for virDomainXMLFlags */ /* Reserve bits 0 for virInterfaceXMLFlags */ ... VIR_NORMALIZE_XML_VALIDATE = (1<< 30), /* perform RNG validation */ } virConnectNormalizeXMLFlags; /** * virConnectNormalizeXML: * @conn: Pointer to connection * @xml: XML to be normalized * @type: one value from virConnectNormalizeXMLTypes, documenting * expected top-level element of @xml * @flags: bitwise-OR of virConnectNormalizeXMLFlags * * This function parses the incoming @xml, with root element * according to @type, and returns the same XML normalized to the * same form it would appear via the corresponding vir*GetXMLDesc * function if it had described a real libvirt object. No * machine state is checked or changed, so while the XML may be * valid, subsequent use of the XML might still fail for other * reasons such as an attempt to reuse a UUID. * * The normalization process may reorder attributes and elements, * and may add new elements describing default states. By default, * unrecognized elements and attributes are silently removed, but * if @flags includes VIR_NORMALIZE_XML_VALIDATE, an error is * raised if any unknown input is encountered. * * Depending on @type, additional bits in @flags may be understood. * For example, if @type specifies a domain or sub-element of a * domain, the flags can include virDomainXMLFlags such as * VIR_DOMAIN_XML_INACTIVE; whereas if @type specifies a host * interface, the flags can include virInterfaceXMLFlags. * * Returns the normalized XML (caller must free), or NULL if the * XML could not be normalized. */ char * virConnectNormalizeXML(virConnectPtr conn, const char *xml, unsigned int type, unsigned int flags);