[libvirt] [PATCH] virsh: Fix a regression of detaching device
Since for node <address>, the values for attrs "domain", "bus", "slot", "function" are all converted to heximal when parsing the XML. However commit ea7182c29 tries to examine if the device represented by the XML from user exists in the domain XML earlier before the XML parsing, and thus if the user use different base to represent the device address, e.g. <address type='pci' domain='0' bus='0' slot='7'/> vshCompleteXMLFromDomain won't find the device, and quit with error "no such device" even if the device does exist. --- tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++------ 1 files changed, 35 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3654589..5c78995 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12107,12 +12107,14 @@ vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) { xmlNodePtr child1, child2; xmlAttrPtr attr; - char *prop1, *prop2; + char *prop1 = NULL; + char *prop2 = NULL; bool found; bool visited; bool ret = false; long n1_child_size, n2_child_size, n1_iter; - virBitmapPtr bitmap; + virBitmapPtr bitmap = NULL; + unsigned int prop1_ui, prop2_ui; if (!n1 && !n2) return true; @@ -12129,13 +12131,36 @@ vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) if (attr->type == XML_ATTRIBUTE_NODE) { prop1 = virXMLPropString(n1, (const char *) attr->name); prop2 = virXMLPropString(n2, (const char *) attr->name); - if (STRNEQ_NULLABLE(prop1, prop2)) { - xmlFree(prop1); - xmlFree(prop2); - return false; + + if (STREQ((const char *)n2->name, "address") && + prop1 && prop2 && + (STREQ((const char *)attr->name, "domain") || + STREQ((const char *)attr->name, "bus") || + STREQ((const char *)attr->name, "slot") || + STREQ((const char *)attr->name, "function"))) { + if (virStrToLong_ui(prop1, NULL, 0, &prop1_ui) < 0) { + vshError(NULL, _("can't parse value for %s"), attr->name); + goto cleanup; + } + + if (virStrToLong_ui(prop2, NULL, 0, &prop2_ui) < 0) { + vshError(NULL, _("can't parse value for %s"), attr->name); + goto cleanup; + } + + if (prop1_ui != prop2_ui) { + goto cleanup; + } + } else { + if (STRNEQ_NULLABLE(prop1, prop2)) { + goto cleanup; + } } + xmlFree(prop1); + prop1 = NULL; xmlFree(prop2); + prop2 = NULL; } attr = attr->next; } @@ -12207,6 +12232,10 @@ vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) ret = true; cleanup: + if (prop1) + xmlFree(prop1); + if (prop2) + xmlFree(prop2); virBitmapFree(bitmap); return ret; } -- 1.7.7.3
On 12/19/2011 04:01 AM, Osier Yang wrote:
Since for node <address>, the values for attrs "domain", "bus", "slot", "function" are all converted to heximal when parsing the XML. However commit ea7182c29 tries to examine if the device represented by the XML from user exists in the domain XML earlier before the XML parsing, and thus if the user use different base to represent the device address, e.g. <address type='pci' domain='0' bus='0' slot='7'/>
vshCompleteXMLFromDomain won't find the device, and quit with error "no such device" even if the device does exist. --- tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++------ 1 files changed, 35 insertions(+), 6 deletions(-)
Hmm. I agree that we have a problem, but I think this patch is taking the wrong approach to fixing it. By special casing which pieces of XML should be parsed as an integer and compared by integer value rather than by string equality, we effectively end up having to add more and more exceptions every time someone else points out another field that needs to be treated as an integer. It would be a lot better to read the user's input, and normalize it into a single form, then compare the normalized forms; rather than this patch's attempt to normalize individual fields during the comparison. But I'm not sure how we would go about doing that, short of reusing our XML parsing code from domain_conf.c which does the normalization by flattening XML into a consistent in-memory representation, followed by the code that re-generates XML from that in-memory representation. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/19/2011 11:38 AM, Eric Blake wrote:
On 12/19/2011 04:01 AM, Osier Yang wrote:
Since for node <address>, the values for attrs "domain", "bus", "slot", "function" are all converted to heximal when parsing the XML. However commit ea7182c29 tries to examine if the device represented by the XML from user exists in the domain XML earlier before the XML parsing, and thus if the user use different base to represent the device address, e.g. <address type='pci' domain='0' bus='0' slot='7'/>
vshCompleteXMLFromDomain won't find the device, and quit with error "no such device" even if the device does exist. --- tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++------ 1 files changed, 35 insertions(+), 6 deletions(-)
Hmm. I agree that we have a problem, but I think this patch is taking the wrong approach to fixing it. By special casing which pieces of XML should be parsed as an integer and compared by integer value rather than by string equality, we effectively end up having to add more and more exceptions every time someone else points out another field that needs to be treated as an integer.
It would be a lot better to read the user's input, and normalize it into a single form, then compare the normalized forms; rather than this patch's attempt to normalize individual fields during the comparison. But I'm not sure how we would go about doing that, short of reusing our XML parsing code from domain_conf.c which does the normalization by flattening XML into a consistent in-memory representation, followed by the code that re-generates XML from that in-memory representation.
Maybe we need a new API, something like: char *virConnectNormalizeXML(virConnectPtr conn, const char *xml, unsigned int flags) that takes incoming xml, passes it through the internal parser, and spits back out the resulting xml (or diagnoses the xml as invalid by returning NULL), all without modifying any existing domain, network, storage pool, or other object managed by xml. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2011年12月20日 02:51, Eric Blake wrote:
On 12/19/2011 11:38 AM, Eric Blake wrote:
On 12/19/2011 04:01 AM, Osier Yang wrote:
Since for node<address>, the values for attrs "domain", "bus", "slot", "function" are all converted to heximal when parsing the XML. However commit ea7182c29 tries to examine if the device represented by the XML from user exists in the domain XML earlier before the XML parsing, and thus if the user use different base to represent the device address, e.g. <address type='pci' domain='0' bus='0' slot='7'/>
vshCompleteXMLFromDomain won't find the device, and quit with error "no such device" even if the device does exist. --- tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++------ 1 files changed, 35 insertions(+), 6 deletions(-)
Hmm. I agree that we have a problem, but I think this patch is taking the wrong approach to fixing it. By special casing which pieces of XML should be parsed as an integer and compared by integer value rather than by string equality, we effectively end up having to add more and more exceptions every time someone else points out another field that needs to be treated as an integer.
Yes, I'd say commit ea7182c29 might be not a quite good idea for we do somethings special internally, just like for the address conversion, it's not quite good idea to do things like early comparison in virsh, but since it's already pushed, the better way is to find some way to work around.
It would be a lot better to read the user's input, and normalize it into a single form, then compare the normalized forms; rather than this patch's attempt to normalize individual fields during the comparison. But I'm not sure how we would go about doing that, short of reusing our XML parsing code from domain_conf.c which does the normalization by flattening XML into a consistent in-memory representation, followed by the code that re-generates XML from that in-memory representation.
Maybe we need a new API, something like:
char *virConnectNormalizeXML(virConnectPtr conn, const char *xml, unsigned int flags)
that takes incoming xml, passes it through the internal parser, and spits back out the resulting xml (or diagnoses the xml as invalid by returning NULL), all without modifying any existing domain, network, storage pool, or other object managed by xml.
Sounds great idea, but the only question in my mind is how useful the API will be. Seems it will be useful only for the app which does things like virsh, or some one wants to see the dry-run XML pieces. And it inspires me perhaps it will be great if we support dry-run XML not only for the XML pieces, but also for the whole domain, network, storage, interface, ... configs too? Regards, Osier
participants (2)
-
Eric Blake -
Osier Yang