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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org