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

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



On 11/07/12 07:48, Doug Goldstein wrote:
On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake 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&lt;2</description>
+  <memory file='d,e'/>
    <disks>
      <disk name='vda' snapshot='external'>
        <source file='a&amp;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 has an automagic behavior where it skips adding anything to the buffer if either of the arguments is NULL. This is used to avoid conditions when creating XML docs.


+    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.

ACK to the patch. Hm, it would be great to have the ability to have auto-generated file names for memory images for external checkpoints unfortunately there's the issue with the location as we don't have any template. I think we are good for now, but we might consider adding a default location of those.


Peter





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