[libvirt] [PATCH] Revert "virsh: Increase device-detach intelligence"
Osier Yang
jyang at redhat.com
Thu Jan 12 01:17:15 UTC 2012
On 2012年01月12日 01:34, Eric Blake wrote:
> On 01/11/2012 04:47 AM, Osier Yang wrote:
>> This reverts commit ea7182c29f185e7c1527b10fbe16dc4ba1f45a88.
>>
>> Conflicts are resolved.
>>
>> This is a temporary reverting for it introduces regression of device
>> detaching (any device XML doesn't uses the same order as libvirt
>> generates, or has uses decimal for some attrs (e.g. "slot") will
>> cause the failure, as virsh compares the XML simply earlier before internal
>> parsing). We could take the patch back when the new API for normalizing
>> XML is ready.
>> ---
>> tools/virsh.c | 292 +++-----------------------------------------------------
>> 1 files changed, 16 insertions(+), 276 deletions(-)
>
> Do we really want to flat-out revert this entire patch?
>
>>
>> -/**
>> - * Check if n1 is superset of n2, meaning n1 contains all elements and
>> - * attributes as n2 at least. Including children.
>> - * @n1 first node
>> - * @n2 second node
>> - * returns true in case n1 covers n2, false otherwise.
>> - */
>> -static bool
>> -vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2)
>> -{
>
> This function seems like we would still be using it if we add the new
> API for normalizing XML; so reverting it just to re-add it later doesn't
> make as much sense as just leaving it in place and not using it for now
> (but you may have to add a compiler attribute to mark it unused in the
> meantime).
Agreed
>
>> -/**
>> - * 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, char *oldXML,
>> - char **newXML)
>
> This was the function where Dan suggested the idea of taking
> (normalized) oldXML, and turning it into a couple of XPath queries of
> the domain XML. For example, if the user passes only a MAC address,
> that would translate into an XPath query of how many interface/mac
> elements exist, and if that is exactly one, then grab the interface node
> that contains the given interface/mac value. So we may be rewriting
> this anyways, at which point it is no different if we keep this function
> around unused or wait for the rewrite.
>
>> @@ -12835,11 +12589,10 @@ static const vshCmdOptDef opts_detach_device[] = {
>> static bool
>> cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
>> {
>> - virDomainPtr dom = NULL;
>> + virDomainPtr dom;
>> const char *from = NULL;
>> - char *buffer = NULL, *new_buffer = NULL;
>> + char *buffer;
>> int ret;
>> - bool funcRet = false;
>> unsigned int flags;
>
> Some of these cleanups, such as setting up funcRet to a default value
> and adding a cleanup label, were independently useful. I'm not sure
> whether reverting the entire patch, or just the portion associated with
> the problems of subset matching, makes more sense. That is,
>
>> -
>> - 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;
>> + virDomainFree(dom);
>> + return false;
>> }
>
> this would be the only hunk that really matters, rather than the entire
> patch, while still keeping all the other improvements of the original,
> and keeping the attempt around for comparing against a future patch that
> does a better comparison job.
Agreed.
>
> I'm inclined to say:
>
> Don't push this patch until we have the replacement ready to go. What
> we have now may not be 100% working, but it does work in some cases,
> while reverting now with no replacement takes us back to 0% working. In
> other words, I think that using this patch in isolation is wrong; and
> would feel more comfortable ack-ing it as part of a larger series that
> replaces things with a better version. There's still time to get such a
> series written before 0.9.10.
>
Agreed.
More information about the libvir-list
mailing list