Re: [libvirt] [PATCHv2 2/4] snapshot: add virsh back-compat support for new filters
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)