[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, Oct 06, 2011 at 05:41:11PM -0600, Eric Blake wrote:
> 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

  ACK, an useful addition !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]