[libvirt] [PATCHv2 2/4] snapshot: add virsh back-compat support for new filters

[Eric Blake]

On 11/19/2012 04:26 AM, Peter Krempa wrote:
> On 11/15/12 00:36, Eric Blake wrote:
>> Snapshot filtering based on types is useful enough to add
>> back-compat support into virsh.  It is also rather easy - all
>> versions of libvirt that don't understand the new filter flags
>> already gave us sufficient information in a single XML field
>> to reconstruct all the information we need (that is, it isn't
>> until libvirt 1.0.1 that we have more interesting types of
>> snapshots, such as offline external).
>>

>> +
>> +cleanup:
>> +    if (ret < 0) {
>> +        vshReportError(ctl);
>> +        vshError(ctl, "%s", _("unable to perform snapshot filtering"));
> 
> Hm, reporting of libvirt's error isn't really needed here. When the
> filtering fails everything should fail gracefully and the error would be
> printed by vshCmdRun at the end when the command fails. The only benefit
> of this is that the error from libvirt is printed before the more
> specific error. In other commands we just print a local error and then
> let the libvirt error to be printed by the command handler.

Indeed - skipping error processing here will let the error status still
exist when the caller exits, so the user will still get a decent
message.  I'm fine deleting it.

> 
>> +    } else {
>> +        vshResetLibvirtError();
>> +    }
>> +    VIR_FREE(state);
>> +    xmlXPathFreeContext(ctxt);
>> +    xmlFreeDoc(xmldoc);
>> +    VIR_FREE(xml);
>> +    return ret;
>> +}
>> +
> 
> ACK when you remove the error message printing or provide a explanation
> why you chose that approach.

Explanation? Merely based on copy-and-paste from vshGetSnapshotParent;
but _that_ function has more complicated calling patterns, so while I
didn't mind cleaning up vshSnapshotFilter, I'm not as sure about
cleaning up the former.

Thanks for the review, and will push shortly.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121119/71646626/attachment-0001.sig>