[libvirt] [PATCH 2/2] virsh: add snapshot-create-as memspec support

[Doug Goldstein]

On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake at redhat.com> wrote:
> External checkpoints could be created with snapshot-create, but
> without libvirt supplying a default name for the memory file,
> it is essential to add a new argument to snapshot-create-as to
> allow the user to choose the memory file name.  This adds the
> option --memspec [file=]name[,snapshot=type], where type can
> be none, internal, or external.  For an example,
>
> virsh snapshot-create-as $dom --memspec /path/to/file
>
> is the shortest possible command line for creating an external
> checkpoint, named after the current timestamp.
>
> * tools/virsh-snapshot.c (vshParseSnapshotMemspec): New function.
> (cmdSnapshotCreateAs): Use it.
> * tests/virsh-optparse (test_url): Test it.
> * tools/virsh.pod (snapshot-create-as): Document it.
> ---
>  tests/virsh-optparse   |  5 +++--
>  tools/virsh-snapshot.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod        | 17 ++++++++++++-----
>  3 files changed, 67 insertions(+), 7 deletions(-)
>
> diff --git a/tests/virsh-optparse b/tests/virsh-optparse
> index 4ddc31a..214ae41 100755
> --- a/tests/virsh-optparse
> +++ b/tests/virsh-optparse
> @@ -1,7 +1,7 @@
>  #!/bin/sh
>  # Ensure that virsh option parsing doesn't regress
>
> -# Copyright (C) 2011 Red Hat, Inc.
> +# Copyright (C) 2011-2012 Red Hat, Inc.
>
>  # This program is free software: you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -73,6 +73,7 @@ done
>  cat <<\EOF > exp-out || framework_failure
>  <domainsnapshot>
>    <description>1<2</description>
> +  <memory file='d,e'/>
>    <disks>
>      <disk name='vda' snapshot='external'>
>        <source file='a&b,c'/>
> @@ -84,7 +85,7 @@ cat <<\EOF > exp-out || framework_failure
>  EOF
>  virsh -q -c $test_url snapshot-create-as --print-xml test \
>    --diskspec 'vda,file=a&b,,c,snapshot=external' --description '1<2' \
> -  --diskspec vdb >out 2>>err || fail=1
> +  --diskspec vdb --memspec file=d,,e >out 2>>err || fail=1
>  compare exp-out out || fail=1
>
>  cat <<\EOF > exp-out || framework_failure
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 159f577..952dec5 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -194,6 +194,49 @@ cleanup:
>   * "snapshot-create-as" command
>   */
>  static int
> +vshParseSnapshotMemspec(vshControl *ctl, virBufferPtr buf, const char *str)
> +{
> +    int ret = -1;
> +    const char *snapshot = NULL;
> +    const char *file = NULL;
> +    char **array = NULL;
> +    int narray;
> +    int i;
> +
> +    if (!str)
> +        return 0;
> +
> +    narray = vshStringToArray(str, &array);
> +    if (narray < 0)
> +        goto cleanup;
> +
> +    for (i = 0; i < narray; i++) {
> +        if (!snapshot && STRPREFIX(array[i], "snapshot="))
> +            snapshot = array[i] + strlen("snapshot=");
> +        else if (!file && STRPREFIX(array[i], "file="))
> +            file = array[i] + strlen("file=");
> +        else if (!file && *array[i] == '/')
> +            file = array[i];
> +        else
> +            goto cleanup;
> +    }
> +
> +    virBufferAddLit(buf, "  <memory");
> +    virBufferEscapeString(buf, " snapshot='%s'", snapshot);

What's this do when a snapshot value wasn't supplied?

> +    virBufferEscapeString(buf, " file='%s'", file);
> +    virBufferAddLit(buf, "/>\n");
> +    ret = 0;
> +cleanup:
> +    if (ret < 0)
> +        vshError(ctl, _("unable to parse memspec: %s"), str);
> +    if (array) {
> +        VIR_FREE(*array);
> +        VIR_FREE(array);
> +    }
> +    return ret;
> +}
> +
> +static int
>  vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
>  {
>      int ret = -1;
> @@ -263,6 +306,8 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
>      {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")},
>      {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")},
>      {"live", VSH_OT_BOOL, 0, N_("take a live snapshot")},
> +    {"memspec", VSH_OT_DATA, VSH_OFLAG_REQ_OPT,
> +     N_("memory attributes: [file=]name[,snapshot=type]")},
>      {"diskspec", VSH_OT_ARGV, 0,
>       N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")},
>      {NULL, 0, 0, NULL}
> @@ -276,6 +321,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
>      char *buffer = NULL;
>      const char *name = NULL;
>      const char *desc = NULL;
> +    const char *memspec = NULL;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      unsigned int flags = 0;
>      const vshCmdOpt *opt = NULL;
> @@ -310,6 +356,12 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
>          virBufferEscapeString(&buf, "  <name>%s</name>\n", name);
>      if (desc)
>          virBufferEscapeString(&buf, "  <description>%s</description>\n", desc);
> +
> +    if (vshCommandOptString(cmd, "memspec", &memspec) < 0 ||
> +        vshParseSnapshotMemspec(ctl, &buf, memspec) < 0) {
> +        virBufferFreeAndReset(&buf);
> +        goto cleanup;
> +    }
>      if (vshCommandOptBool(cmd, "diskspec")) {
>          virBufferAddLit(&buf, "  <disks>\n");
>          while ((opt = vshCommandOptArgv(cmd, opt))) {
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 4ec9f2e..3f9b3ea 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2670,8 +2670,8 @@ by command such as B<destroy> or by internal guest action).
>
>  =item B<snapshot-create-as> I<domain> {[I<--print-xml>]
>  | [I<--no-metadata>] [I<--halt>] [I<--reuse-external>]} [I<name>]
> -[I<description>] [I<--disk-only> [I<--quiesce>] [I<--atomic>]
> -[I<--live>] [[I<--diskspec>] B<diskspec>]...]
> +[I<description>] [I<--disk-only> [I<--quiesce>]] [I<--atomic>]
> +[[I<--live>] [I<--memspec> B<memspec>]] [I<--diskspec>] B<diskspec>]...
>
>  Create a snapshot for domain I<domain> with the given <name> and
>  <description>; if either value is omitted, libvirt will choose a
> @@ -2681,9 +2681,16 @@ Otherwise, if I<--halt> is specified, the domain will be left in an
>  inactive state after the snapshot is created, and if I<--disk-only>
>  is specified, the snapshot will not include vm state.
>
> -The I<--disk-only> flag is used to request a disk-only snapshot.  When
> -this flag is in use, the command can also take additional I<diskspec>
> -arguments to add <disk> elements to the xml.  Each <diskspec> is in the
> +The I<--memspec> option can be used to control whether a checkpoint
> +is internal or external.  The I<--memspec> flag is mandatory, followed
> +by a B<memspec> of the form B<[file=]name[,snapshot=type]>, where
> +type can be B<none>, B<internal>, or B<external>.  To include a literal
> +comma in B<file=name>, escape it with a second comma.
> +
> +The I<--diskspec> option can be used to control how I<--disk-only> and
> +external checkpoints create external files.  This option can occur
> +multiple times, according to the number of <disk> elements in the domain
> +xml.  Each <diskspec> is in the
>  form B<disk[,snapshot=type][,driver=type][,file=name]>.  To include a
>  literal comma in B<disk> or in B<file=name>, escape it with a second
>  comma.  A literal I<--diskspec> must precede each B<diskspec> unless
> --
> 1.7.11.7

Just one small question. I haven't compiled it or syntax checked it
but visually it looks good.


-- 
Doug Goldstein