[libvirt] [RFC PATCH] snapshot: better virsh handling of missing current, parent
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). tools/virsh.c | 54 +++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 39 insertions(+), 15 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 1909dce..179cda5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12945,6 +12945,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) char *xml = NULL; const char *snapshotname = NULL; unsigned int flags = 0; + const char *domname; if (vshCommandOptBool(cmd, "security-info")) flags |= VIR_DOMAIN_XML_SECURE; @@ -12952,7 +12953,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; - dom = vshCommandOptDomain(ctl, cmd, NULL); + dom = vshCommandOptDomain(ctl, cmd, &domname); if (dom == NULL) goto cleanup; @@ -12986,9 +12987,12 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) } current = virDomainHasCurrentSnapshot(dom, 0); - if (current < 0) + if (current < 0) { + goto cleanup; + } else if (!current) { + vshError(ctl, _("domain '%s' has no current snapshot"), domname); goto cleanup; - else if (current) { + } else { const char *name = NULL; if (!(snapshot = virDomainSnapshotCurrent(dom, 0))) @@ -13010,6 +13014,8 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + if (!ret) + virshReportError(ctl); VIR_FREE(xml); if (snapshot) virDomainSnapshotFree(snapshot); @@ -13020,26 +13026,33 @@ cleanup: } /* Helper function to get the name of a snapshot's parent. Caller - * must free the result. */ -static char * -vshGetSnapshotParent(vshControl *ctl, virDomainSnapshotPtr snapshot) + * must free the result. Returns 0 on success (including when it was + * proven no parent exists), and -1 on failure with error reported + * (such as no snapshot support or domain deleted in meantime). */ +static int +vshGetSnapshotParent(vshControl *ctl, virDomainSnapshotPtr snapshot, + char **parent_name) { virDomainSnapshotPtr parent = NULL; char *xml = NULL; xmlDocPtr xmldoc = NULL; xmlXPathContextPtr ctxt = NULL; - char *parent_name = NULL; + int ret = -1; + + *parent_name = NULL; /* Try new API, since it is faster. */ if (!ctl->useSnapshotGetXML) { parent = virDomainSnapshotGetParent(snapshot, 0); if (parent) { - /* API works, and virDomainSnapshotGetName will succeed */ - parent_name = vshStrdup(ctl, virDomainSnapshotGetName(parent)); + /* API works, and virDomainSnapshotGetName will be accurate */ + *parent_name = vshStrdup(ctl, virDomainSnapshotGetName(parent)); + ret = 0; goto cleanup; } if (last_error->code == VIR_ERR_NO_DOMAIN_SNAPSHOT) { /* API works, and we found a root with no parent */ + ret = 0; goto cleanup; } /* API didn't work, fall back to XML scraping. */ @@ -13054,15 +13067,23 @@ vshGetSnapshotParent(vshControl *ctl, virDomainSnapshotPtr snapshot) if (!xmldoc) goto cleanup; - parent_name = virXPathString("string(/domainsnapshot/parent/name)", ctxt); + *parent_name = virXPathString("string(/domainsnapshot/parent/name)", ctxt); + ret = 0; cleanup: + if (ret < 0) { + virshReportError(ctl); + vshError(ctl, "%s", _("unable to determine if snapshot has parent")); + } else { + virFreeError(last_error); + last_error = NULL; + } if (parent) virDomainSnapshotFree(parent); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc); VIR_FREE(xml); - return parent_name; + return ret; } /* @@ -13187,13 +13208,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (snapshot) virDomainSnapshotFree(snapshot); snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); - if (!snapshot) { + if (!snapshot || + vshGetSnapshotParent(ctl, snapshot, &parents[i]) < 0) { while (--i >= 0) VIR_FREE(parents[i]); VIR_FREE(parents); goto cleanup; } - parents[i] = vshGetSnapshotParent(ctl, snapshot); } for (i = 0 ; i < actual ; i++) { memset(indentBuf, '\0', sizeof indentBuf); @@ -13389,9 +13410,12 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) if (snapshot == NULL) goto cleanup; - parent = vshGetSnapshotParent(ctl, snapshot); - if (!parent) + if (vshGetSnapshotParent(ctl, snapshot, &parent) < 0) goto cleanup; + if (!parent) { + vshError(ctl, _("snapshot '%s' has no parent"), name); + goto cleanup; + } vshPrint(ctl, "%s", parent); -- 1.7.4.4
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 :-)
On 10/04/2011 12:54 PM, Laine Stump wrote:
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.
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 :-)
I went ahead and pushed this as-is, on the basis that a common usage pattern, virsh snapshot-revert dom $(virsh snapshot-current --name), will do the right thing when there is no stdout, whether or not stderr was supplied, but having stderr available can help understand the situation when using the command in isolation. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump