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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list