Re: [libvirt] [PATCH 14/7] snapshot: virsh shorthand for operating on current snap

Rather than having to do: $ virsh snapshot-revert dom $(virsh snapshot-current dom --name) I thought it would be nice to do: $ virsh snaphot-revert dom --current I intentionally made it so that omitting both --current and a name is an error (having the absence of a name imply --current seems just a bit too magic, so --current must be explicit). I didn't add 'virsh snapshot-dumpxml --current' since we already have 'virsh snapshot-current' for the same task. All other snapshot-* options that take a snapshotname now take --current in its place; while keeping snapshot-edit backwards-compatible. * tools/virsh.c (vshLookupSnapshot): New helper function. (cmdSnapshotEdit, cmdSnapshotList, cmdSnapshotParent) (cmdSnapshotDelete, cmdDomainSnapshotRevert): Use it, adding an option where needed. * tools/virsh.pod (snapshot-delete, snapshot-edit) (snapshot-list, snapshot-parent, snapshot-revert): Document use of --current. (snapshot-dumpxml): Mention alternative. --- Does not strictly depend on virDomainSnapshotNumChildren, other than the fact that I'm touching 'virsh snapshot-list dom --from name' to add 'virsh snapshot-list dom --current'; I could split that change out from the rest of the patch if desired to get the ease-of-use in the other snapshot-* commands before the new API is approved. tools/virsh.c | 109 ++++++++++++++++++++++++++++++++++++------------------- tools/virsh.pod | 31 ++++++++++------ 2 files changed, 90 insertions(+), 50 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 86b230d..ea7b56b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12816,6 +12816,48 @@ cleanup: return ret; } +/* Helper for resolving {--current | --ARG name} into a snapshot + * belonging to DOM. If EXCLUSIVE, fail if both --current and arg are + * present. On success, populate *SNAP, and if NAME is not NULL, + * *NAME, before returning 0. On failure, return -1 after issuing an + * error message. */ +static int +vshLookupSnapshot(vshControl *ctl, const vshCmd *cmd, + const char *arg, bool exclusive, virDomainPtr dom, + virDomainSnapshotPtr *snap, const char **name) +{ + bool current = vshCommandOptBool(cmd, "current"); + const char *snapname = NULL; + + if (vshCommandOptString(cmd, arg, &snapname) < 0) { + vshError(ctl, _("invalid argument for --%s"), arg); + return -1; + } + + if (exclusive && current && snapname) { + vshError(ctl, _("--%s and --current are mutually exclusive"), arg); + return -1; + } + + if (snapname) { + *snap = virDomainSnapshotLookupByName(dom, snapname, 0); + } else if (current) { + *snap = virDomainSnapshotCurrent(dom, 0); + } else { + vshError(ctl, _("--%s or --current is required"), arg); + return -1; + } + if (!*snap) { + virshReportError(ctl); + return -1; + } + + if (name && *snap) + *name = vshStrdup(ctl, virDomainSnapshotGetName(*snap)); + + return 0; +} + /* * "snapshot-edit" command */ @@ -12827,7 +12869,7 @@ static const vshCmdInfo info_snapshot_edit[] = { static const vshCmdOptDef opts_snapshot_edit[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("snapshot name")}, {"current", VSH_OT_BOOL, 0, N_("also set edited snapshot as current")}, {NULL, 0, 0, NULL} }; @@ -12845,21 +12887,19 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE; unsigned int define_flags = VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; - if (vshCommandOptBool(cmd, "current")) + if (vshCommandOptBool(cmd, "current") && + vshCommandOptBool(cmd, "snapshotname")) define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) - goto cleanup; - dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) goto cleanup; - snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", false, dom, + &snapshot, &name) < 0) goto cleanup; /* Get the XML configuration of the snapshot. */ @@ -13109,6 +13149,8 @@ static const vshCmdOptDef opts_snapshot_list[] = { N_("list only snapshots that have metadata that would prevent undefine")}, {"tree", VSH_OT_BOOL, 0, N_("list snapshots in a tree")}, {"from", VSH_OT_DATA, 0, N_("limit list to children of given snapshot")}, + {"current", VSH_OT_BOOL, 0, + N_("limit list to children of current snapshot")}, {"descendants", VSH_OT_BOOL, 0, N_("with --from, list all descendants")}, {NULL, 0, 0, NULL} }; @@ -13142,10 +13184,17 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) int start_index = -1; bool descendants = false; - if (vshCommandOptString(cmd, "from", &from) < 0) { - vshError(ctl, _("invalid from argument '%s'"), from); + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + dom = vshCommandOptDomain(ctl, cmd, NULL); + if (dom == NULL) + goto cleanup; + + if ((vshCommandOptBool(cmd, "from") || + vshCommandOptBool(cmd, "current")) && + vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from) < 0) goto cleanup; - } if (vshCommandOptBool(cmd, "parent")) { if (vshCommandOptBool(cmd, "roots")) { @@ -13176,18 +13225,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; } - if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; - - dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; - if (from) { descendants = vshCommandOptBool(cmd, "descendants"); - start = virDomainSnapshotLookupByName(dom, from, 0); - if (!start) - goto cleanup; if (descendants || tree) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } @@ -13519,7 +13558,8 @@ static const vshCmdInfo info_snapshot_parent[] = { static const vshCmdOptDef opts_snapshot_parent[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("find parent of snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("find parent of current snapshot")}, {NULL, 0, 0, NULL} }; @@ -13539,11 +13579,8 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) - goto cleanup; - - snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", true, dom, + &snapshot, &name) < 0) goto cleanup; if (vshGetSnapshotParent(ctl, snapshot, &parent) < 0) @@ -13578,7 +13615,8 @@ static const vshCmdInfo info_snapshot_revert[] = { static const vshCmdOptDef opts_snapshot_revert[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("revert to current snapshot")}, {"running", VSH_OT_BOOL, 0, N_("after reverting, change state to running")}, {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")}, {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")}, @@ -13614,11 +13652,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) - goto cleanup; - - snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", true, dom, + &snapshot, &name) < 0) goto cleanup; result = virDomainRevertToSnapshot(snapshot, flags); @@ -13654,7 +13689,8 @@ static const vshCmdInfo info_snapshot_delete[] = { static const vshCmdOptDef opts_snapshot_delete[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("delete current snapshot")}, {"children", VSH_OT_BOOL, 0, N_("delete snapshot and all children")}, {"children-only", VSH_OT_BOOL, 0, N_("delete children but not snapshot")}, {"metadata", VSH_OT_BOOL, 0, @@ -13678,7 +13714,8 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", true, dom, + &snapshot, &name) < 0) goto cleanup; if (vshCommandOptBool(cmd, "children")) @@ -13688,10 +13725,6 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "metadata")) flags |= VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY; - snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) - goto cleanup; - /* XXX If we wanted, we could emulate DELETE_CHILDREN_ONLY even on * older servers that reject the flag, by manually computing the * list of descendants. But that's a lot of code to maintain. */ diff --git a/tools/virsh.pod b/tools/virsh.pod index dd60a64..e1a9b86 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1951,11 +1951,13 @@ the XML. With I<snapshotname>, this is a request to make the existing named snapshot become the current snapshot, without reverting the domain. -=item B<snapshot-edit> I<domain> I<snapshotname> [I<--current>] +=item B<snapshot-edit> I<domain> [I<snapshotname>] [I<--current>] Edit the XML configuration file for I<snapshotname> of a domain. If -I<--current> is specified, also force the edited snapshot to become -the current snapshot. +both I<snapshotname> and I<--current> are specified, also force the +edited snapshot to become the current snapshot. If I<snapshotname> +is omitted, then I<--current> must be supplied, to edit the current +snapshot. This is equivalent to: @@ -1969,7 +1971,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] -[I<--metadata>] [[I<--from>] B<snapshot> [I<--descendants>]] +[I<--metadata>] [{[I<--from>] B<snapshot> | I<--current>} [I<--descendants>]] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -1981,7 +1983,8 @@ If I<--tree> is specified, the output will be in a tree format, listing just snapshot names. These three options are mutually exclusive. If I<--from> is provided, filter the list to snapshots which are -children of the given B<snapshot>. When used in isolation or with +children of the given B<snapshot>; or if I<--current> is provided, +start at the current snapshot. When used in isolation or with I<--parent>, the list is limited to direct children unless I<--descendants> is also present. When used with I<--tree>, the use of I<--descendants> is implied. This option is not compatible @@ -1996,15 +1999,18 @@ a transient domain. Output the snapshot XML for the domain's snapshot named I<snapshot>. Using I<--security-info> will also include security sensitive information. +Use B<snapshot-current> to easily access the XML of the current snapshot. -=item B<snapshot-parent> I<domain> I<snapshot> +=item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>} -Output the name of the parent snapshot for the given I<snapshot>, if any. +Output the name of the parent snapshot, if any, for the given +I<snapshot>, or for the current snapshot with I<--current>. -=item B<snapshot-revert> I<domain> I<snapshot> [{I<--running> | I<--paused>}] -[I<--force>] +=item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} +[{I<--running> | I<--paused>}] [I<--force>] -Revert the given domain to the snapshot specified by I<snapshot>. Be aware +Revert the given domain to the snapshot specified by I<snapshot>, or to +the current snapshot with I<--current>. Be aware that this is a destructive action; any changes in the domain since the last snapshot was taken will be lost. Also note that the state of the domain after snapshot-revert is complete will be the state of the domain at the time @@ -2034,10 +2040,11 @@ snapshot that uses a provably incompatible configuration, as well as with an inactive snapshot that is combined with the I<--start> or I<--pause> flag. -=item B<snapshot-delete> I<domain> I<snapshot> [I<--metadata>] +=item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>} [I<--metadata>] [{I<--children> | I<--children-only>}] -Delete the snapshot for the domain named I<snapshot>. If this snapshot +Delete the snapshot for the domain named I<snapshot>, or the current +snapshot with I<--current>. If this snapshot has child snapshots, changes from this snapshot will be merged into the children. If I<--children> is passed, then delete this snapshot and any children of this snapshot. If I<--children-only> is passed, then delete