On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake(a)redhat.com&gt; wrote: 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(). 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. 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

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); + 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

On 11/07/12 07:48, Doug Goldstein wrote: 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