[libvirt] [PATCH 2/2] virsh: add snapshot-create-as memspec support
Peter Krempa
pkrempa at redhat.com
Wed Nov 7 11:30:26 UTC 2012
On 11/07/12 07:48, Doug Goldstein wrote:
> 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 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
>
>
More information about the libvir-list
mailing list