[libvirt] [RFC PATCH] snapshot: better virsh handling of missing current, parent

[Laine Stump]

On 09/30/2011 12:19 PM, Eric Blake wrote:
> Previously, virsh 'snapshot-parent' and 'snapshot-current' were
> completely silent in the case where the code conclusively proved
> there was no parent or current snapshot, but differed in exit
> status; this silence caused some confusion on whether the commands
> worked.  Furthermore, commit d1be48f introduced a regression where
> snapshot-parent would leak output about an unknown function, but
> only on the first attempt, when talking to an older server that
> lacks virDomainSnapshotGetParent.  This changes things to consistenly
> report an error message and exit with status 1 when no snapshot
> exists, and to avoid leaking unknown function warnings when using
> fallbacks.
>
> * tools/virsh.c (vshGetSnapshotParent): Alter signature, to
> distinguish between real error and missing parent.  Don't pollute
> last_error on success.
> (cmdSnapshotParent): Adjust caller.  Always output message on
> failure.
> (vshGetSnapshotParent): Adjust caller.
> (cmdSnapshotCurrent): Always output message on failure.
> ---
>
> This patch is an RFC because of backwards-compatibility concerns:
>
> Currently, snapshot-current outputs nothing but has status 0 if
> there is no current snapshot; but snapshot-parent outputs nothing
> but has status 1 if there is no parent snapshot.  Either way, we
> have an inconsistency that needs to be fixed, and gaining consistency
> means breaking backwards compatibility with at least one command.
>
> Approach 1 (this patch): change snapshot-parent and snapshot-current
> to always output something, whether to stdout on success or to
> stderr on failure, with lack of a snapshot being considered a
> failure (where snapshot-current used to treat it as success).
>
> Approach 2 (not written) since snapshot-current existed much longer,
> makesnapshot-parent consistent by giving status 0 when it is silent.
>
> Preferences?  (I guess mine is approach 1, by evidence of this patch).


The code all looks fine, so ACK to that. Not being very familiar with 
the snapshot stuff, I can't say that I have an valid opinion on whether 
it's better to log an error or exit silently when there is no parent. I 
guess I would defer to your opinion, since you're the person who's spent 
the most time dealing with snapshots lately :-)