[libvirt] [PATCH] Revert "virsh: Increase device-detach intelligence"

[Osier Yang]

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.