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