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 :-)