On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake@redhat.com> wrote:

So far, none of the existing callers of vshStringToArray expected the user to ever pass a literal comma; meanwhile, snapshot parsing had rolled its own array parser. Moving the comma escaping into the common function won't affect any existing callers, and will make this function reusable for adding memory handling to snapshot parsing.

As a bonus, the testsuite was already testing snapshot parsing, so the fact that the test still passes means that we are now giving testsuite exposure to vshStringToArray.

* tools/virsh-snapshot.c (vshParseSnapshotDiskspec): Move ,, parsing... * tools/virsh.c (vshStringToArray): ...into common function. Also, vshStrdup can't fail. --- tools/virsh-snapshot.c | 50 ++++++++++++++++++++++------------------------- tools/virsh.c | 53 ++++++++++++++++++++++++++++++-------------------- 2 files changed, 55 insertions(+), 48 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 818ef56..159f577 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -197,33 +197,26 @@ static int vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str) { int ret = -1; - char *name = NULL; - char *snapshot = NULL; - char *driver = NULL; - char *file = NULL; - char *spec = vshStrdup(ctl, str); - char *tmp = spec; - size_t len = strlen(str); - - if (*str == ',') + const char *name = NULL; + const char *snapshot = NULL; + const char *driver = NULL; + const char *file = NULL; + char **array = NULL; + int narray; + int i; + + narray = vshStringToArray(str, &array); + if (narray <= 0) goto cleanup; - name = tmp; - while ((tmp = strchr(tmp, ','))) { - if (tmp[1] == ',') { - /* Recognize ,, as an escape for a literal comma */ - memmove(&tmp[1], &tmp[2], len - (tmp - spec) - 2 + 1); - len--; - tmp++; - continue; - } - /* Terminate previous string, look for next recognized one */ - *tmp++ = '\0'; - if (!snapshot && STRPREFIX(tmp, "snapshot=")) - snapshot = tmp + strlen("snapshot="); - else if (!driver && STRPREFIX(tmp, "driver=")) - driver = tmp + strlen("driver="); - else if (!file && STRPREFIX(tmp, "file=")) - file = tmp + strlen("file="); + + name = array[0]; + for (i = 1; i < narray; i++) { + if (!snapshot && STRPREFIX(array[i], "snapshot=")) + snapshot = array[i] + strlen("snapshot="); + else if (!driver && STRPREFIX(array[i], "driver=")) + driver = array[i] + strlen("driver="); + else if (!file && STRPREFIX(array[i], "file=")) + file = array[i] + strlen("file="); else goto cleanup; } @@ -245,7 +238,10 @@ vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str) cleanup: if (ret < 0) vshError(ctl, _("unable to parse diskspec: %s"), str); - VIR_FREE(spec); + if (array) { + VIR_FREE(*array); + VIR_FREE(array); + } return ret; }

diff --git a/tools/virsh.c b/tools/virsh.c index a5585e1..9598b75 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -179,35 +179,46 @@ vshStringToArray(const char *str, { char *str_copied = vshStrdup(NULL, str); char *str_tok = NULL; + char *tmp; unsigned int nstr_tokens = 0; char **arr = NULL; + size_t len = strlen(str_copied);

If str_copied is NULL due to an OOM (I think that's what vshStrdup does on OOM) or if someone was bad with their call to vshStringToArray() we'll go boom on this strlen().

/* tokenize the string from user and save it's parts into an array */ - if (str_copied) { - nstr_tokens = 1; - - /* count the delimiters */ - str_tok = str_copied; - while (*str_tok) { - if (*str_tok == ',') - nstr_tokens++; + nstr_tokens = 1; + + /* count the delimiters, recognizing ,, as an escape for a + * literal comma */ + str_tok = str_copied; + while ((str_tok = strchr(str_tok, ','))) { + if (str_tok[1] == ',') str_tok++; - } + else + nstr_tokens++; + str_tok++; + }

- if (VIR_ALLOC_N(arr, nstr_tokens) < 0) { - virReportOOMError(); - VIR_FREE(str_copied); - return -1; - } + if (VIR_ALLOC_N(arr, nstr_tokens) < 0) { + virReportOOMError(); + VIR_FREE(str_copied); + return -1; + }

- /* tokenize the input string */ - nstr_tokens = 0; - str_tok = str_copied; - do { - arr[nstr_tokens] = strsep(&str_tok, ","); - nstr_tokens++; - } while (str_tok); + /* tokenize the input string */ + nstr_tokens = 0; + tmp = str_tok = str_copied; + while ((tmp = strchr(tmp, ','))) { + if (tmp[1] == ',') { + memmove(&tmp[1], &tmp[2], len - (tmp - str_copied) - 2 + 1); + len--; + tmp++;

Not a review but just trying to understand it (maybe a short comment above for others like me that needed to pause to look at this would be nice) but basically you're just shifting off one of the double commas right? The code above looks correct for that.

+ continue; + } + *tmp++ = '\0'; + arr[nstr_tokens++] = str_tok; + str_tok = tmp; } + arr[nstr_tokens++] = str_tok;

*array = arr; return nstr_tokens; -- 1.7.11.7

Just 1 little nit for the strlen(), which its possible we guarantee we won't hit that. And then a clarification. But otherwise this looks good so visual ACK. I haven't pulled it down and compiled it or syntax checked it. -- Doug Goldstein

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