On Fri, Sep 30, 2011 at 05:09:23PM -0600, Eric Blake wrote:

The previous API addition allowed traversal up the hierarchy; this one makes it easier to traverse down the hierarchy.

In the python bindings, virDomainSnapshotNumChildren can be generated, but virDomainSnapshotListChildrenNames had to copy from the hand-written example of virDomainSnapshotListNames.

* include/libvirt/libvirt.h.in (virDomainSnapshotNumChildren) (virDomainSnapshotListChildrenNames): New prototypes. (VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS): New flag alias. * src/libvirt.c (virDomainSnapshotNumChildren) (virDomainSnapshotListChildrenNames): New functions. * src/libvirt_public.syms: Export them. * src/driver.h (virDrvDomainSnapshotNumChildren) (virDrvDomainSnapshotListChildrenNames): New callbacks. * python/generator.py (skip_impl, nameFixup): Update lists. * python/libvirt-override-api.xml: Likewise. * python/libvirt-override.c (libvirt_virDomainSnapshotListChildrenNames): New wrapper function. ---

v2: no change

include/libvirt/libvirt.h.in | 27 +++++++-- python/generator.py | 4 ++ python/libvirt-override-api.xml | 12 +++- python/libvirt-override.c | 45 ++++++++++++++++ src/driver.h | 12 ++++ src/libvirt.c | 111 +++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + 7 files changed, 204 insertions(+), 9 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a832b65..7403a9a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2686,13 +2686,19 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags);

-/* Flags valid for both virDomainSnapshotNum() and - * virDomainSnapshotListNames(). */ +/* Flags valid for virDomainSnapshotNum(), + * virDomainSnapshotListNames(), virDomainSnapshotNumChildren(), and + * virDomainSnapshotListChildrenNames(). Note that the interpretation + * of flag (1<<0) depends on which function it is passed to. */ typedef enum { - VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots which - have no parents */ - VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots which - have metadata */ + VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots + with no parents, when + listing a domain */ + VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS = (1 << 0), /* List all descendants, + not just children, when + listing a snapshot */ + VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots + which have metadata */ } virDomainSnapshotListFlags;

Hum, okay, a tad bit confusing though

/* Return the number of snapshots for this domain */ @@ -2702,6 +2708,15 @@ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags); int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, unsigned int flags);

+/* Return the number of child snapshots for this snapshot */ +int virDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, + unsigned int flags); + +/* Get the names of all child snapshots for this snapshot */ +int virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, + char **names, int nameslen, + unsigned int flags); + /* Get a handle to a named snapshot */ virDomainSnapshotPtr virDomainSnapshotLookupByName(virDomainPtr domain, const char *name,

Okay

diff --git a/python/generator.py b/python/generator.py index 79558dd..71afdb7 100755 --- a/python/generator.py +++ b/python/generator.py @@ -352,6 +352,7 @@ skip_impl = ( 'virConnectListDefinedInterfaces', 'virConnectListNWFilters', 'virDomainSnapshotListNames', + 'virDomainSnapshotListChildrenNames', 'virConnGetLastError', 'virGetLastError', 'virDomainGetInfo', @@ -963,6 +964,9 @@ def nameFixup(name, classe, type, file): elif name[0:26] == "virDomainSnapshotListNames": func = name[9:] func = string.lower(func[0:1]) + func[1:] + elif name[0:28] == "virDomainSnapshotNumChildren": + func = name[17:] + func = string.lower(func[0:1]) + func[1:] elif name[0:20] == "virDomainSnapshotNum": func = name[9:] func = string.lower(func[0:1]) + func[1:] diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 3013e46..ef02f34 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -346,14 +346,20 @@ <function name='virDomainSnapshotListNames' file='python'> <info>collect the list of snapshots for the given domain</info> <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> - <arg name='flags' type='unsigned int' info='flags, curently unused'/> - <return type='str *' info='the list of Names of None in case of error'/> + <arg name='flags' type='unsigned int' info='flags'/> + <return type='str *' info='the list of Names or None in case of error'/> + </function> + <function name='virDomainSnapshotListChildrenNames' file='python'> + <info>collect the list of child snapshots for the given snapshot</info> + <arg name='snapshot' type='virDomainSnapshotPtr' info='pointer to the snapshot'/> + <arg name='flags' type='unsigned int' info='flags'/> + <return type='str *' info='the list of Names or None in case of error'/> </function> <function name='virDomainRevertToSnapshot' file='python'> <info>revert the domain to the given snapshot</info> <arg name='dom' type='virDomainPtr' info='dummy domain pointer'/> <arg name='snap' type='virDomainSnapshotPtr' info='pointer to the snapshot'/> - <arg name='flags' type='unsigned int' info='flags, curently unused'/> + <arg name='flags' type='unsigned int' info='flags'/> <return type='int' info="0 on success, -1 on error"/> </function> <function name='virDomainGetBlockJobInfo' file='python'> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index d65423d..523c03b 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1727,6 +1727,51 @@ libvirt_virDomainSnapshotListNames(PyObject *self ATTRIBUTE_UNUSED, }

static PyObject * +libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) { + PyObject *py_retval; + char **names = NULL; + int c_retval, i; + virDomainSnapshotPtr snap; + PyObject *pyobj_snap; + unsigned int flags; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListChildrenNames", &pyobj_snap, &flags)) + return(NULL); + snap = (virDomainSnapshotPtr) PyvirDomainSnapshot_Get(pyobj_snap); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainSnapshotNumChildren(snap, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (c_retval) { + names = malloc(sizeof(*names) * c_retval); + if (!names) + return VIR_PY_NONE; + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainSnapshotListChildrenNames(snap, names, c_retval, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) { + free(names); + return VIR_PY_NONE; + } + } + py_retval = PyList_New(c_retval); + + if (names) { + for (i = 0;i < c_retval;i++) { + PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i])); + free(names[i]); + } + free(names); + } + + return(py_retval); +} + +static PyObject * libvirt_virDomainRevertToSnapshot(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { int c_retval; diff --git a/src/driver.h b/src/driver.h index f85a1b1..b899d0e 100644 --- a/src/driver.h +++ b/src/driver.h @@ -584,6 +584,16 @@ typedef int int nameslen, unsigned int flags);

+typedef int + (*virDrvDomainSnapshotNumChildren)(virDomainSnapshotPtr snapshot, + unsigned int flags); + +typedef int + (*virDrvDomainSnapshotListChildrenNames)(virDomainSnapshotPtr snapshot, + char **names, + int nameslen, + unsigned int flags); + typedef virDomainSnapshotPtr (*virDrvDomainSnapshotLookupByName)(virDomainPtr domain, const char *name, @@ -860,6 +870,8 @@ struct _virDriver { virDrvDomainSnapshotGetXMLDesc domainSnapshotGetXMLDesc; virDrvDomainSnapshotNum domainSnapshotNum; virDrvDomainSnapshotListNames domainSnapshotListNames; + virDrvDomainSnapshotNumChildren domainSnapshotNumChildren; + virDrvDomainSnapshotListChildrenNames domainSnapshotListChildrenNames; virDrvDomainSnapshotLookupByName domainSnapshotLookupByName; virDrvDomainHasCurrentSnapshot domainHasCurrentSnapshot; virDrvDomainSnapshotGetParent domainSnapshotGetParent; diff --git a/src/libvirt.c b/src/libvirt.c index 7b1d953..c2aabf7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16068,6 +16068,117 @@ error: }

/** + * virDomainSnapshotNumChildren: + * @snapshot: a domain snapshot object + * @flags: bitwise-or of supported virDomainSnapshotListFlags + * + * Provides the number of child snapshots for this domain snapshot. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, then the result + * includes all descendants, otherwise it is limited to direct children. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is + * the number of snapshots that also include metadata that would prevent + * the removal of the last reference to a domain; this value will either + * be 0 or the same value as if the flag were not given. + * + * Returns the number of domain snapshots found or -1 in case of error. + */ +int +virDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("snapshot=%p, flags=%x", snapshot, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { + virLibDomainSnapshotError(VIR_ERR_INVALID_DOMAIN_SNAPSHOT, + __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = snapshot->domain->conn; + if (conn->driver->domainSnapshotNumChildren) { + int ret = conn->driver->domainSnapshotNumChildren(snapshot, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +error: + virDispatchError(conn); + return -1; +} + +/** + * virDomainSnapshotListChildrenNames: + * @snapshot: a domain snapshot object + * @names: array to collect the list of names of snapshots + * @nameslen: size of @names + * @flags: bitwise-or of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots that are children of the given + * snapshot, and store their names in @names. Caller is responsible for + * freeing each member of the array. The value to use for @nameslen can + * be determined by virDomainSnapshotNumChildren() with the same @flags. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, then the result + * includes all descendants, otherwise it is limited to direct children. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is + * the number of snapshots that also include metadata that would prevent + * the removal of the last reference to a domain; this value will either + * be 0 or the same value as if the flag were not given. + * + * Returns the number of domain snapshots found or -1 in case of error. + */ +int +virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, + char **names, int nameslen, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("snapshot=%p, names=%p, nameslen=%d, flags=%x", + snapshot, names, nameslen, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { + virLibDomainSnapshotError(VIR_ERR_INVALID_DOMAIN_SNAPSHOT, + __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = snapshot->domain->conn; + + if ((names == NULL) || (nameslen < 0)) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->domainSnapshotListChildrenNames) { + int ret = conn->driver->domainSnapshotListChildrenNames(snapshot, + names, + nameslen, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainSnapshotLookupByName: * @domain: a domain object * @name: name for the domain snapshot diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index afea29b..9762fc4 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -493,6 +493,8 @@ LIBVIRT_0.9.7 { global: virDomainReset; virDomainSnapshotGetParent; + virDomainSnapshotListChildrenNames; + virDomainSnapshotNumChildren; } LIBVIRT_0.9.5;

# .... define new API here using predicted next version number ....

ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Sep 30, 2011 at 05:09:24PM -0600, Eric Blake wrote:

Sometimes, we only care about one branch of the snapshot hierarchy. Make it easier to list a single branch, by using the new APIs.

Technically, I could emulate these new virsh options on old servers by doing a complete dump, then scraping xml to filter out just the snapshots that I care about, but I didn't want to do that in this patch.

* tools/virsh.c (cmdSnapshotList): Add --from, --descendants. * tools/virsh.pod (snapshot-list): Document them. ---

v2: minor rebase cleanups

tools/virsh.c | 76 ++++++++++++++++++++++++++++++++++++++++++++----------- tools/virsh.pod | 9 ++++++- 2 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 955b8df..adafe86 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13102,6 +13102,8 @@ static const vshCmdOptDef opts_snapshot_list[] = { {"metadata", VSH_OT_BOOL, 0, N_("list only snapshots that have metadata that would prevent undefine")}, {"tree", VSH_OT_BOOL, 0, N_("list snapshots in a tree")}, + {"from", VSH_OT_DATA, 0, N_("limit list to children of given snapshot")}, + {"descendants", VSH_OT_BOOL, 0, N_("with --from, list all descendants")}, {NULL, 0, 0, NULL} };

@@ -13128,25 +13130,36 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char timestr[100]; struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); + const char *from = NULL; + virDomainSnapshotPtr start = NULL; + + if (vshCommandOptString(cmd, "from", &from) < 0) { + vshError(ctl, _("invalid from argument '%s'"), from); + goto cleanup; + }

if (vshCommandOptBool(cmd, "parent")) { if (vshCommandOptBool(cmd, "roots")) { vshError(ctl, "%s", - _("--parent and --roots are mutually exlusive")); + _("--parent and --roots are mutually exclusive")); return false; } if (tree) { vshError(ctl, "%s", - _("--parent and --tree are mutually exlusive")); + _("--parent and --tree are mutually exclusive")); return false; } parent_filter = 1; } else if (vshCommandOptBool(cmd, "roots")) { if (tree) { vshError(ctl, "%s", - _("--roots and --tree are mutually exlusive")); + _("--roots and --tree are mutually exclusive")); return false; } + if (from) { + vshError(ctl, "%s", + _("--roots and --from are mutually exclusive")); + } flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; }

@@ -13161,16 +13174,29 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;

- numsnaps = virDomainSnapshotNum(dom, flags); - - /* Fall back to simulation if --roots was unsupported. */ - if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && - (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { - virFreeError(last_error); - last_error = NULL; - parent_filter = -1; - flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + if (from) { + start = virDomainSnapshotLookupByName(dom, from, 0); + if (!start) + goto cleanup; + if (vshCommandOptBool(cmd, "descendants") || tree) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + } + numsnaps = virDomainSnapshotNumChildren(start, flags); + if (numsnaps >= 0 && tree) + numsnaps++; + /* XXX Is it worth emulating --from on older servers? */ + } else { numsnaps = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + parent_filter = -1; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + numsnaps = virDomainSnapshotNum(dom, flags); + } }

if (numsnaps < 0) @@ -13196,14 +13222,32 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (VIR_ALLOC_N(names, numsnaps) < 0) goto cleanup;

- actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); + if (from) { + /* When mixing --from and --tree, we want to start the tree at the + * given snapshot. Without --tree, only list the children. */ + if (tree) { + if (numsnaps) + actual = virDomainSnapshotListChildrenNames(start, names + 1, + numsnaps - 1, + flags); + if (actual >= 0) { + actual++; + names[0] = vshStrdup(ctl, from); + } + } else { + actual = virDomainSnapshotListChildrenNames(start, names, + numsnaps, flags); + } + } else { + actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); + } if (actual < 0) goto cleanup;

if (tree) { char indentBuf[INDENT_BUFLEN]; - char **parents = vshMalloc(ctl, sizeof(char *) * actual); - for (i = 0; i < actual; i++) { + char **parents = vshCalloc(ctl, sizeof(char *), actual); + for (i = (from ? 1 : 0); i < actual; i++) { /* free up memory from previous iterations of the loop */ if (snapshot) virDomainSnapshotFree(snapshot); @@ -13298,6 +13342,8 @@ cleanup: VIR_FREE(state); if (snapshot) virDomainSnapshotFree(snapshot); + if (start) + virDomainSnapshotFree(start); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); diff --git a/tools/virsh.pod b/tools/virsh.pod index 1f7c76f..dd60a64 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1969,7 +1969,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>.

=item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] -[I<--metadata>] +[I<--metadata>] [[I<--from>] B<snapshot> [I<--descendants>]]

List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -1980,6 +1980,13 @@ the list will be filtered to just snapshots that have no parents. If I<--tree> is specified, the output will be in a tree format, listing just snapshot names. These three options are mutually exclusive.

+If I<--from> is provided, filter the list to snapshots which are +children of the given B<snapshot>. When used in isolation or with +I<--parent>, the list is limited to direct children unless +I<--descendants> is also present. When used with I<--tree>, the +use of I<--descendants> is implied. This option is not compatible +with I<--roots>. + If I<--metadata> is specified, the list will be filtered to just snapshots that involve libvirt metadata, and thus would prevent B<undefine> of a persistent domain, or be lost on B<destroy> of

ACK, I'm unsure about the emulation need, except for a server running 0.9.6 and a newer client ... Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Sep 30, 2011 at 05:09:25PM -0600, Eric Blake wrote:

Emulating --from requires grabbing the entire list of snapshots and their parents, and recursively iterating over the list from the point of interest - but we already do that for --tree. This turns on emulation for that situation.

* tools/virsh.c (__vshControl): Rename member. (vshReconnect, cmdConnect, vshGetSnapshotParent): Update clients. (cmdSnapshotList): Add fallback. ---

v2: reuse existing ctl flag, so that virsh in shell mode doesn't repeatedly try non-working commands

tools/virsh.c | 43 +++++++++++++++++++++++++++++-------------- 1 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index adafe86..e3bc364 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -246,8 +246,8 @@ typedef struct __vshControl { char *historyfile; /* readline history file name */ bool useGetInfo; /* must use virDomainGetInfo, since virDomainGetState is not supported */ - bool useSnapshotGetXML; /* must use virDomainSnapshotGetXMLDesc, since - virDomainSnapshotGetParent is missing */ + bool useSnapshotOld; /* cannot use virDomainSnapshotGetParent or + virDomainSnapshotNumChildren */ } __vshControl;

typedef struct vshCmdGrp { @@ -599,7 +599,7 @@ vshReconnect(vshControl *ctl) vshError(ctl, "%s", _("Reconnected to the hypervisor")); disconnected = 0; ctl->useGetInfo = false; - ctl->useSnapshotGetXML = false; + ctl->useSnapshotOld = false; }

/* --------------- @@ -747,7 +747,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) ctl->name = vshStrdup(ctl, name);

ctl->useGetInfo = false; - ctl->useSnapshotGetXML = false; + ctl->useSnapshotOld = false; ctl->readonly = ro;

ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, @@ -13042,7 +13042,7 @@ vshGetSnapshotParent(vshControl *ctl, virDomainSnapshotPtr snapshot, *parent_name = NULL;

/* Try new API, since it is faster. */ - if (!ctl->useSnapshotGetXML) { + if (!ctl->useSnapshotOld) { parent = virDomainSnapshotGetParent(snapshot, 0); if (parent) { /* API works, and virDomainSnapshotGetName will be accurate */ @@ -13056,7 +13056,7 @@ vshGetSnapshotParent(vshControl *ctl, virDomainSnapshotPtr snapshot, goto cleanup; } /* API didn't work, fall back to XML scraping. */ - ctl->useSnapshotGetXML = true; + ctl->useSnapshotOld = true; }

xml = virDomainSnapshotGetXMLDesc(snapshot, 0); @@ -13181,10 +13181,22 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "descendants") || tree) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } - numsnaps = virDomainSnapshotNumChildren(start, flags); - if (numsnaps >= 0 && tree) - numsnaps++; - /* XXX Is it worth emulating --from on older servers? */ + numsnaps = ctl->useSnapshotOld ? -1 : + virDomainSnapshotNumChildren(start, flags); + /* XXX Is it worth emulating --from without --tree on older servers? */ + if (tree) { + if (numsnaps >= 0) { + numsnaps++; + } else if (ctl->useSnapshotOld || + last_error->code == VIR_ERR_NO_SUPPORT) { + /* We can emulate --tree --from. */ + virFreeError(last_error); + last_error = NULL; + ctl->useSnapshotOld = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + numsnaps = virDomainSnapshotNum(dom, flags); + } + } } else { numsnaps = virDomainSnapshotNum(dom, flags);

@@ -13199,8 +13211,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } }

- if (numsnaps < 0) + if (numsnaps < 0) { + if (!last_error) + vshError(ctl, _("missing support")); goto cleanup; + }

if (!tree) { if (parent_filter > 0) @@ -13222,7 +13237,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (VIR_ALLOC_N(names, numsnaps) < 0) goto cleanup;

- if (from) { + if (from && !ctl->useSnapshotOld) { /* When mixing --from and --tree, we want to start the tree at the * given snapshot. Without --tree, only list the children. */ if (tree) { @@ -13247,7 +13262,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { char indentBuf[INDENT_BUFLEN]; char **parents = vshCalloc(ctl, sizeof(char *), actual); - for (i = (from ? 1 : 0); i < actual; i++) { + for (i = (from && !ctl->useSnapshotOld); i < actual; i++) { /* free up memory from previous iterations of the loop */ if (snapshot) virDomainSnapshotFree(snapshot); @@ -13262,7 +13277,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } for (i = 0 ; i < actual ; i++) { memset(indentBuf, '\0', sizeof indentBuf); - if (parents[i] == NULL) + if (ctl->useSnapshotOld ? STREQ(names[i], from) : !parents[i]) cmdNodeListDevicesPrint(ctl, names, parents,

Ah, if it's there already :-) okay then, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Sep 30, 2011 at 05:09:26PM -0600, Eric Blake wrote:

Iterating over one level of children requires parsing all snapshots and their parents; a bit of code shuffling makes it pretty easy to do this as well.

* tools/virsh.c (cmdSnapshotList): Add another fallback. ---

v2: fix compilation error, rebase around ctl flag

tools/virsh.c | 48 ++++++++++++++++++++++++++++++++---------------- 1 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index e3bc364..b2523d3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13117,6 +13117,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) information needed, 1 for parent column */ int numsnaps; char **names = NULL; + char **parents = NULL; int actual = 0; int i; xmlDocPtr xml = NULL; @@ -13132,6 +13133,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) bool tree = vshCommandOptBool(cmd, "tree"); const char *from = NULL; virDomainSnapshotPtr start = NULL; + bool descendants = false;

if (vshCommandOptString(cmd, "from", &from) < 0) { vshError(ctl, _("invalid from argument '%s'"), from); @@ -13175,27 +13177,29 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) goto cleanup;

if (from) { + descendants = vshCommandOptBool(cmd, "descendants"); start = virDomainSnapshotLookupByName(dom, from, 0); if (!start) goto cleanup; - if (vshCommandOptBool(cmd, "descendants") || tree) { + if (descendants || tree) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } numsnaps = ctl->useSnapshotOld ? -1 : virDomainSnapshotNumChildren(start, flags); - /* XXX Is it worth emulating --from without --tree on older servers? */ - if (tree) { - if (numsnaps >= 0) { - numsnaps++; - } else if (ctl->useSnapshotOld || - last_error->code == VIR_ERR_NO_SUPPORT) { - /* We can emulate --tree --from. */ + if (numsnaps < 0) { + /* XXX also want to emulate --descendants without --tree */ + if ((!descendants || tree) && + (ctl->useSnapshotOld || + last_error->code == VIR_ERR_NO_SUPPORT)) { + /* We can emulate --from. */ virFreeError(last_error); last_error = NULL; ctl->useSnapshotOld = true; flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; numsnaps = virDomainSnapshotNum(dom, flags); } + } else if (tree) { + numsnaps++; } } else { numsnaps = virDomainSnapshotNum(dom, flags); @@ -13259,9 +13263,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (actual < 0) goto cleanup;

- if (tree) { - char indentBuf[INDENT_BUFLEN]; - char **parents = vshCalloc(ctl, sizeof(char *), actual); + if (!tree) + qsort(&names[0], actual, sizeof(char*), namesorter); + + if (tree || ctl->useSnapshotOld) { + parents = vshCalloc(ctl, sizeof(char *), actual); for (i = (from && !ctl->useSnapshotOld); i < actual; i++) { /* free up memory from previous iterations of the loop */ if (snapshot) @@ -13275,6 +13281,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) goto cleanup; } } + } + if (tree) { + char indentBuf[INDENT_BUFLEN]; for (i = 0 ; i < actual ; i++) { memset(indentBuf, '\0', sizeof indentBuf); if (ctl->useSnapshotOld ? STREQ(names[i], from) : !parents[i]) @@ -13288,16 +13297,19 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) 0, indentBuf); } - for (i = 0 ; i < actual ; i++) - VIR_FREE(parents[i]); - VIR_FREE(parents);

ret = true; goto cleanup; } else { - qsort(&names[0], actual, sizeof(char*), namesorter); + if (ctl->useSnapshotOld && descendants) { + /* XXX emulate --descendants as well */ + goto cleanup; + }

for (i = 0; i < actual; i++) { + if (ctl->useSnapshotOld && STRNEQ_NULLABLE(parents[i], from)) + continue; + /* free up memory from previous iterations of the loop */ VIR_FREE(parent); VIR_FREE(state); @@ -13362,9 +13374,13 @@ cleanup: xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - for (i = 0; i < actual; i++) + for (i = 0; i < actual; i++) { VIR_FREE(names[i]); + if (parents) + VIR_FREE(parents[i]); + } VIR_FREE(names); + VIR_FREE(parents); if (dom) virDomainFree(dom);

Okay, getting parents global to the function as well as associated cleanup looks a bit simpler actually. ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Sep 30, 2011 at 05:09:28PM -0600, Eric Blake wrote:

Very mechanical. I'm so glad we've automated the generation of things, compared to what it was in 0.8.x days, where this would be much longer.

* src/remote/remote_protocol.x (REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN) (REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES): New rpcs. (remote_domain_snapshot_num_children_args) (remote_domain_snapshot_num_children_ret) (remote_domain_snapshot_list_children_names_args) (remote_domain_snapshot_list_children_names_ret): New structs. * src/remote/remote_driver.c (remote_driver): Use it. * src/remote_protocol-structs: Update. ---

v2: fix typo in remote_protocol-structs

src/remote/remote_driver.c | 2 ++ src/remote/remote_protocol.x | 25 +++++++++++++++++++++++-- src/remote_protocol-structs | 20 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 83f4f3c..0e303df 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4411,6 +4411,8 @@ static virDriver remote_driver = { .domainSnapshotGetXMLDesc = remoteDomainSnapshotGetXMLDesc, /* 0.8.0 */ .domainSnapshotNum = remoteDomainSnapshotNum, /* 0.8.0 */ .domainSnapshotListNames = remoteDomainSnapshotListNames, /* 0.8.0 */ + .domainSnapshotNumChildren = remoteDomainSnapshotNumChildren, /* 0.9.7 */ + .domainSnapshotListChildrenNames = remoteDomainSnapshotListChildrenNames, /* 0.9.7 */ .domainSnapshotLookupByName = remoteDomainSnapshotLookupByName, /* 0.8.0 */ .domainHasCurrentSnapshot = remoteDomainHasCurrentSnapshot, /* 0.8.0 */ .domainSnapshotGetParent = remoteDomainSnapshotGetParent, /* 0.9.7 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index c8a92fd..f95253e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2067,6 +2067,25 @@ struct remote_domain_snapshot_list_names_ret { remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ };

+struct remote_domain_snapshot_num_children_args { + remote_nonnull_domain_snapshot snap; + unsigned int flags; +}; + +struct remote_domain_snapshot_num_children_ret { + int num; +}; + +struct remote_domain_snapshot_list_children_names_args { + remote_nonnull_domain_snapshot snap; + int maxnames; + unsigned int flags; +}; + +struct remote_domain_snapshot_list_children_names_ret { + remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ +}; + struct remote_domain_snapshot_lookup_by_name_args { remote_nonnull_domain dom; remote_nonnull_string name; @@ -2524,8 +2543,10 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB = 241, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_SPEED = 242, /* autogen autogen */ REMOTE_PROC_DOMAIN_BLOCK_STATS_FLAGS = 243, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, /* autogen autogen */ - REMOTE_PROC_DOMAIN_RESET = 245 /* autogen autogen */ + REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, /* autogen autogen priority:high */ + REMOTE_PROC_DOMAIN_RESET = 245, /* autogen autogen */ + REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN = 246, /* autogen autogen priority:high */ + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247 /* autogen autogen priority:high */

/* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 69175cc..7894441 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1557,6 +1557,24 @@ struct remote_domain_snapshot_list_names_ret { remote_nonnull_string * names_val; } names; }; +struct remote_domain_snapshot_num_children_args { + remote_nonnull_domain_snapshot snap; + u_int flags; +}; +struct remote_domain_snapshot_num_children_ret { + int num; +}; +struct remote_domain_snapshot_list_children_names_args { + remote_nonnull_domain_snapshot snap; + int maxnames; + u_int flags; +}; +struct remote_domain_snapshot_list_children_names_ret { + struct { + u_int names_len; + remote_nonnull_string * names_val; + } names; +}; struct remote_domain_snapshot_lookup_by_name_args { remote_nonnull_domain dom; remote_nonnull_string name; @@ -1971,4 +1989,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BLOCK_STATS_FLAGS = 243, REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, REMOTE_PROC_DOMAIN_RESET = 245, + REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN = 246, + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, };

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Fri, Sep 30, 2011 at 05:09:29PM -0600, Eric Blake wrote:

Not too hard to wire up. The trickiest part is realizing that listing children of a snapshot cannot use SNAPSHOT_LIST_ROOTS, and that we overloaded that bit to also mean SNAPSHOT_LIST_DESCENDANTS; we use that bit to decide which iteration to use, but don't want the existing counting/listing functions to see that bit.

* src/conf/domain_conf.h (virDomainSnapshotObjListNumFrom) (virDomainSnapshotObjListGetNamesFrom): New prototypes. * src/conf/domain_conf.c (virDomainSnapshotObjListNumFrom) (virDomainSnapshotObjListGetNamesFrom): New functions. * src/libvirt_private.syms (domain_conf.h): Export them. * src/qemu/qemu_driver.c (qemuDomainSnapshotNumChildren) (qemuDomainSnapshotListChildrenNames): New functions. ---

v2: no change, but now virsh changes have been tested both with and without this patch

src/conf/domain_conf.c | 51 +++++++++++++++++++++++++++ src/conf/domain_conf.h | 7 ++++ src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 0 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c141982..438e3b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12138,6 +12138,37 @@ cleanup: return -1; }

+int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, + virDomainSnapshotObjListPtr snapshots, + char **const names, int maxnames, + unsigned int flags) +{ + struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; + int i; + + data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) + virDomainSnapshotForEachDescendant(snapshots, snapshot, + virDomainSnapshotObjListCopyNames, + &data); + else + virDomainSnapshotForEachChild(snapshots, snapshot, + virDomainSnapshotObjListCopyNames, &data); + + if (data.oom) { + virReportOOMError(); + goto cleanup; + } + + return data.numnames; + +cleanup: + for (i = 0; i < data.numnames; i++) + VIR_FREE(data.names[i]); + return -1; +} + struct virDomainSnapshotNumData { int count; unsigned int flags; @@ -12165,6 +12196,26 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, return data.count; }

+int +virDomainSnapshotObjListNumFrom(virDomainSnapshotObjPtr snapshot, + virDomainSnapshotObjListPtr snapshots, + unsigned int flags) +{ + struct virDomainSnapshotNumData data = { 0, 0 }; + + data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) + virDomainSnapshotForEachDescendant(snapshots, snapshot, + virDomainSnapshotObjListCount, + &data); + else + virDomainSnapshotForEachChild(snapshots, snapshot, + virDomainSnapshotObjListCount, &data); + + return data.count; +} + virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0bc0042..1258740 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1488,6 +1488,13 @@ int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, unsigned int flags); int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, unsigned int flags); +int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, + virDomainSnapshotObjListPtr snapshots, + char **const names, int maxnames, + unsigned int flags); +int virDomainSnapshotObjListNumFrom(virDomainSnapshotObjPtr snapshot, + virDomainSnapshotObjListPtr snapshots, + unsigned int flags); virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name); void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c2a3fab..6b9ceaa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -413,7 +413,9 @@ virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotHasChildren; virDomainSnapshotObjListGetNames; +virDomainSnapshotObjListGetNamesFrom; virDomainSnapshotObjListNum; +virDomainSnapshotObjListNumFrom; virDomainSnapshotObjListRemove; virDomainSnapshotStateTypeFromString; virDomainSnapshotStateTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a171cf..48b0b22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9441,6 +9441,91 @@ cleanup: return n; }

+static int +qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, + char **names, + int nameslen, + unsigned int flags) +{ + struct qemud_driver *driver = snapshot->domain->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainSnapshotObjPtr snap = NULL; + int n = -1; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(snapshot->domain->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + if (!snap) { + qemuReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, + _("no domain snapshot with matching name '%s'"), + snapshot->name); + goto cleanup; + } + + n = virDomainSnapshotObjListGetNamesFrom(snap, &vm->snapshots, + names, nameslen, flags); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return n; +} + +static int +qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, + unsigned int flags) +{ + struct qemud_driver *driver = snapshot->domain->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainSnapshotObjPtr snap = NULL; + int n = -1; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(snapshot->domain->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + if (!snap) { + qemuReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, + _("no domain snapshot with matching name '%s'"), + snapshot->name); + goto cleanup; + } + + /* All qemu snapshots have libvirt metadata, so + * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our + * answer. */ + + n = virDomainSnapshotObjListNumFrom(snap, &vm->snapshots, flags); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return n; +} + static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, const char *name, unsigned int flags) @@ -10558,6 +10643,8 @@ static virDriver qemuDriver = { .domainSnapshotGetXMLDesc = qemuDomainSnapshotGetXMLDesc, /* 0.8.0 */ .domainSnapshotNum = qemuDomainSnapshotNum, /* 0.8.0 */ .domainSnapshotListNames = qemuDomainSnapshotListNames, /* 0.8.0 */ + .domainSnapshotNumChildren = qemuDomainSnapshotNumChildren, /* 0.9.7 */ + .domainSnapshotListChildrenNames = qemuDomainSnapshotListChildrenNames, /* 0.9.7 */ .domainSnapshotLookupByName = qemuDomainSnapshotLookupByName, /* 0.8.0 */ .domainHasCurrentSnapshot = qemuDomainHasCurrentSnapshot, /* 0.8.0 */ .domainSnapshotGetParent = qemuDomainSnapshotGetParent, /* 0.9.7 */

ACK, I would tend to be a bit more defensive myself byt not dereferencing 3 layers of pointer as the first instructions in the function, but if you're confident the structures can't be missing :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

No need to request the parent of a snapshot if we aren't going to use it. * src/esx/esx_vi.c (esxVI_GetSnapshotTreeByName): Make parent optional. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML) (esxDomainSnapshotLookupByName, esxDomainRevertToSnapshot) (esxDomainSnapshotDelete): Simplify accordingly. --- src/esx/esx_driver.c | 12 ++++-------- src/esx/esx_vi.c | 7 ++++--- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 9718f61..3f26557 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4228,7 +4228,6 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, esxVI_ObjectContent *virtualMachine = NULL; esxVI_VirtualMachineSnapshotTree *rootSnapshotList = NULL; esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; - esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL; esxVI_ManagedObjectReference *task = NULL; esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; @@ -4259,7 +4258,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, esxVI_LookupRootSnapshotTreeList(priv->primary, domain->uuid, &rootSnapshotList) < 0 || esxVI_GetSnapshotTreeByName(rootSnapshotList, def->name, - &snapshotTree, &snapshotTreeParent, + &snapshotTree, NULL, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -4437,7 +4436,6 @@ esxDomainSnapshotLookupByName(virDomainPtr domain, const char *name, esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; - esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL; virDomainSnapshotPtr snapshot = NULL; virCheckFlags(0, NULL); @@ -4449,7 +4447,7 @@ esxDomainSnapshotLookupByName(virDomainPtr domain, const char *name, if (esxVI_LookupRootSnapshotTreeList(priv->primary, domain->uuid, &rootSnapshotTreeList) < 0 || esxVI_GetSnapshotTreeByName(rootSnapshotTreeList, name, &snapshotTree, - &snapshotTreeParent, + NULL, esxVI_Occurrence_RequiredItem) < 0) { goto cleanup; } @@ -4567,7 +4565,6 @@ esxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) esxPrivate *priv = snapshot->domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotList = NULL; esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; - esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL; esxVI_ManagedObjectReference *task = NULL; esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; @@ -4581,7 +4578,7 @@ esxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) if (esxVI_LookupRootSnapshotTreeList(priv->primary, snapshot->domain->uuid, &rootSnapshotList) < 0 || esxVI_GetSnapshotTreeByName(rootSnapshotList, snapshot->name, - &snapshotTree, &snapshotTreeParent, + &snapshotTree, NULL, esxVI_Occurrence_RequiredItem) < 0) { goto cleanup; } @@ -4621,7 +4618,6 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) esxPrivate *priv = snapshot->domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotList = NULL; esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; - esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL; esxVI_Boolean removeChildren = esxVI_Boolean_False; esxVI_ManagedObjectReference *task = NULL; esxVI_TaskInfoState taskInfoState; @@ -4641,7 +4637,7 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) if (esxVI_LookupRootSnapshotTreeList(priv->primary, snapshot->domain->uuid, &rootSnapshotList) < 0 || esxVI_GetSnapshotTreeByName(rootSnapshotList, snapshot->name, - &snapshotTree, &snapshotTreeParent, + &snapshotTree, NULL, esxVI_Occurrence_RequiredItem) < 0) { goto cleanup; } diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index fa26dea..4a8c709 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -2243,7 +2243,7 @@ esxVI_GetSnapshotTreeByName esxVI_VirtualMachineSnapshotTree *candidate; if (snapshotTree == NULL || *snapshotTree != NULL || - snapshotTreeParent == NULL || *snapshotTreeParent != NULL) { + (snapshotTreeParent && *snapshotTreeParent != NULL)) { ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); return -1; } @@ -2252,14 +2252,15 @@ esxVI_GetSnapshotTreeByName candidate = candidate->_next) { if (STREQ(candidate->name, name)) { *snapshotTree = candidate; - *snapshotTreeParent = NULL; + if (snapshotTreeParent) + *snapshotTreeParent = NULL; return 1; } if (esxVI_GetSnapshotTreeByName(candidate->childSnapshotList, name, snapshotTree, snapshotTreeParent, occurrence) > 0) { - if (*snapshotTreeParent == NULL) { + if (snapshotTreeParent && *snapshotTreeParent == NULL) { *snapshotTreeParent = candidate; } -- 1.7.4.4

It was fairly trivial to return snapshot listing based on a point in the hierarchy, rather than starting at all roots. * src/esx/esx_driver.c (esxDomainSnapshotNumChildren) (esxDomainSnapshotListChildrenNames): New functions. --- src/esx/esx_driver.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 99 insertions(+), 0 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 3f26557..bcf2406 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4429,6 +4429,103 @@ esxDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, +static int +esxDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags) +{ + int count = -1; + esxPrivate *priv = snapshot->domain->conn->privateData; + esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; + esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; + bool recurse; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); + + recurse = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0; + + if (esxVI_EnsureSession(priv->primary) < 0) { + return -1; + } + + if (esxVI_LookupRootSnapshotTreeList(priv->primary, snapshot->domain->uuid, + &rootSnapshotTreeList) < 0 || + esxVI_GetSnapshotTreeByName(rootSnapshotTreeList, snapshot->name, + &snapshotTree, NULL, + esxVI_Occurrence_RequiredItem) < 0) { + goto cleanup; + } + + /* ESX snapshots do not require libvirt to maintain any metadata. */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) { + count = 0; + goto cleanup; + } + + count = esxVI_GetNumberOfSnapshotTrees(snapshotTree->childSnapshotList, + recurse); + +cleanup: + esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotTreeList); + + return count; +} + + + +static int +esxDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, + char **names, int nameslen, + unsigned int flags) +{ + int result = -1; + esxPrivate *priv = snapshot->domain->conn->privateData; + esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; + esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL; + bool recurse; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); + + recurse = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0; + + if (names == NULL || nameslen < 0) { + ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); + return -1; + } + + if (nameslen == 0) { + return 0; + } + + if (esxVI_EnsureSession(priv->primary) < 0) { + return -1; + } + + if (esxVI_LookupRootSnapshotTreeList(priv->primary, snapshot->domain->uuid, + &rootSnapshotTreeList) < 0 || + esxVI_GetSnapshotTreeByName(rootSnapshotTreeList, snapshot->name, + &snapshotTree, NULL, + esxVI_Occurrence_RequiredItem) < 0) { + goto cleanup; + } + + /* ESX snapshots do not require libvirt to maintain any metadata. */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) { + result = 0; + goto cleanup; + } + + result = esxVI_GetSnapshotTreeNames(snapshotTree->childSnapshotList, + names, nameslen, recurse); + +cleanup: + esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotTreeList); + + return result; +} + + + static virDomainSnapshotPtr esxDomainSnapshotLookupByName(virDomainPtr domain, const char *name, unsigned int flags) @@ -4874,6 +4971,8 @@ static virDriver esxDriver = { .domainSnapshotGetXMLDesc = esxDomainSnapshotGetXMLDesc, /* 0.8.0 */ .domainSnapshotNum = esxDomainSnapshotNum, /* 0.8.0 */ .domainSnapshotListNames = esxDomainSnapshotListNames, /* 0.8.0 */ + .domainSnapshotNumChildren = esxDomainSnapshotNumChildren, /* 0.9.7 */ + .domainSnapshotListChildrenNames = esxDomainSnapshotListChildrenNames, /* 0.9.7 */ .domainSnapshotLookupByName = esxDomainSnapshotLookupByName, /* 0.8.0 */ .domainHasCurrentSnapshot = esxDomainHasCurrentSnapshot, /* 0.8.0 */ .domainSnapshotGetParent = esxDomainSnapshotGetParent, /* 0.9.7 */ -- 1.7.4.4

2011/10/4 Eric Blake <eblake@redhat.com>:

Older VBox required grabbing all snapshots, then looking through them until a name match was found.  But when VBox 3.1 introduced snapshot branching, it also added the ability to lookup a snapshot by name instead of UUID; exploit this for faster snapshot lookup.

* src/vbox/vbox_tmpl.c (vboxDomainSnapshotGet): Newer vbox added snapshot lookup by name. ---

Caveat - this is written solely by reading VBox documentation, and I was only able to compile test.  I have no idea if this really works, or if VBox 3.1 is the right cut-off point.

 src/vbox/vbox_tmpl.c |   31 ++++++++++++++++++++++++++++++-  1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 2eb23fb..ba2252c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5586,9 +5586,12 @@ vboxDomainSnapshotGet(vboxGlobalData *data,                       IMachine *machine,                       const char *name)  { -    ISnapshot **snapshots = NULL;     ISnapshot *snapshot = NULL;     nsresult rc; + +#if VBOX_API_VERSION < 3001 + +    ISnapshot **snapshots = NULL;     int count = 0;     int i;

@@ -5615,6 +5618,30 @@ vboxDomainSnapshotGet(vboxGlobalData *data,             break;     }

+#else /* VBOX_API_VERSION >= 3001 */ +    PRUnichar *nameUtf16 = NULL; + +    VBOX_UTF8_TO_UTF16(name, &nameUtf16); +    if (!nameUtf16) { +        virReportOOMError(); +        goto cleanup; +    } + +# if VBOX_API_VERSION < 4000 +    rc = machine->vtbl->GetSnapshot(machine, nameUtf16, &snapshot); +# else /* VBOX_API_VERSION >= 4000 */ +    rc = machine->vtbl->FindSnapshot(machine, nameUtf16, &snapshot); +# endif /* VBOX_API_VERSION >= 4000 */

GetSnapshot with a name is wrong. According to the docs GetSnapshot takes an UUID and FindSnapshot takes a name. GetSnapshot also takes NULL as UUID and returns the first snapshot then. GetSnapshot and FindSnapshot have been there since version 2.2 and probably earlier. In version 4.0 FindSnapshot and GetSnapshot have been merged into FindSnapshot that now takes a UUID or a name. So as you're doing a name lookup here GetSnapshot is wrong and FindSnapshot has to be used independent from the API version. I only tested this with VBox 4.0 and it worked, but it'll fail for VBox < 4.0 I think, because of the use of GetSnapshot. As FindSnapshot is available on all VBox versions this function should probably use it unconditional and not fallback to vboxDomainSnapshotGetAll for version < 3.1. But as stated I didn't test this yet.

+    VBOX_UTF16_FREE(nameUtf16); +    if (NS_FAILED(rc)) { +        vboxError(VIR_ERR_INTERNAL_ERROR, +                  _("could not get root snapshot for domain %s"),

This error message it wrong, in this branch you never tried to get the root.

+                  dom->name); +        goto cleanup; +    } + +#endif /* VBOX_API_VERSION >= 3001 */ +     if (!snapshot) {         vboxError(VIR_ERR_OPERATION_INVALID,

While at it, this error code is wrong, it should be VIR_ERR_NO_DOMAIN_SNAPSHOT. Therefore, I suggest this diff on top of this patch to fix the mentioned problems. ACK, with this diff applied. diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 901799b..666d59d 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5627,23 +5627,16 @@ vboxDomainSnapshotGet(vboxGlobalData *data, goto cleanup; } -# if VBOX_API_VERSION < 4000 - rc = machine->vtbl->GetSnapshot(machine, nameUtf16, &snapshot); -# else /* VBOX_API_VERSION >= 4000 */ rc = machine->vtbl->FindSnapshot(machine, nameUtf16, &snapshot); -# endif /* VBOX_API_VERSION >= 4000 */ VBOX_UTF16_FREE(nameUtf16); if (NS_FAILED(rc)) { - vboxError(VIR_ERR_INTERNAL_ERROR, - _("could not get root snapshot for domain %s"), - dom->name); - goto cleanup; + snapshot = NULL; } #endif /* VBOX_API_VERSION >= 3001 */ if (!snapshot) { - vboxError(VIR_ERR_OPERATION_INVALID, + vboxError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("domain %s has no snapshots with name %s"), dom->name, name); goto cleanup; -- Matthias Bolte http://photron.blogspot.com

On Thu, Oct 06, 2011 at 05:41:11PM -0600, Eric Blake wrote:

Rather than having to do:

$ virsh snapshot-revert dom $(virsh snapshot-current dom --name)

I thought it would be nice to do:

$ virsh snaphot-revert dom --current

I intentionally made it so that omitting both --current and a name is an error (having the absence of a name imply --current seems just a bit too magic, so --current must be explicit).

I didn't add 'virsh snapshot-dumpxml --current' since we already have 'virsh snapshot-current' for the same task. All other snapshot-* options that take a snapshotname now take --current in its place; while keeping snapshot-edit backwards-compatible.

* tools/virsh.c (vshLookupSnapshot): New helper function. (cmdSnapshotEdit, cmdSnapshotList, cmdSnapshotParent) (cmdSnapshotDelete, cmdDomainSnapshotRevert): Use it, adding an option where needed. * tools/virsh.pod (snapshot-delete, snapshot-edit) (snapshot-list, snapshot-parent, snapshot-revert): Document use of --current. (snapshot-dumpxml): Mention alternative. ---

Does not strictly depend on virDomainSnapshotNumChildren, other than the fact that I'm touching 'virsh snapshot-list dom --from name' to add 'virsh snapshot-list dom --current'; I could split that change out from the rest of the patch if desired to get the ease-of-use in the other snapshot-* commands before the new API is approved.

tools/virsh.c | 109 ++++++++++++++++++++++++++++++++++++------------------- tools/virsh.pod | 31 ++++++++++------ 2 files changed, 90 insertions(+), 50 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 86b230d..ea7b56b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12816,6 +12816,48 @@ cleanup: return ret; }

+/* Helper for resolving {--current | --ARG name} into a snapshot + * belonging to DOM. If EXCLUSIVE, fail if both --current and arg are + * present. On success, populate *SNAP, and if NAME is not NULL, + * *NAME, before returning 0. On failure, return -1 after issuing an + * error message. */ +static int +vshLookupSnapshot(vshControl *ctl, const vshCmd *cmd, + const char *arg, bool exclusive, virDomainPtr dom, + virDomainSnapshotPtr *snap, const char **name) +{ + bool current = vshCommandOptBool(cmd, "current"); + const char *snapname = NULL; + + if (vshCommandOptString(cmd, arg, &snapname) < 0) { + vshError(ctl, _("invalid argument for --%s"), arg); + return -1; + } + + if (exclusive && current && snapname) { + vshError(ctl, _("--%s and --current are mutually exclusive"), arg); + return -1; + } + + if (snapname) { + *snap = virDomainSnapshotLookupByName(dom, snapname, 0); + } else if (current) { + *snap = virDomainSnapshotCurrent(dom, 0); + } else { + vshError(ctl, _("--%s or --current is required"), arg); + return -1; + } + if (!*snap) { + virshReportError(ctl); + return -1; + } + + if (name && *snap) + *name = vshStrdup(ctl, virDomainSnapshotGetName(*snap)); + + return 0; +} + /* * "snapshot-edit" command */ @@ -12827,7 +12869,7 @@ static const vshCmdInfo info_snapshot_edit[] = {

static const vshCmdOptDef opts_snapshot_edit[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("snapshot name")}, {"current", VSH_OT_BOOL, 0, N_("also set edited snapshot as current")}, {NULL, 0, 0, NULL} }; @@ -12845,21 +12887,19 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE; unsigned int define_flags = VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;

- if (vshCommandOptBool(cmd, "current")) + if (vshCommandOptBool(cmd, "current") && + vshCommandOptBool(cmd, "snapshotname")) define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT;

if (!vshConnectionUsability(ctl, ctl->conn)) return false;

- if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) - goto cleanup; - dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) goto cleanup;

- snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", false, dom, + &snapshot, &name) < 0) goto cleanup;

/* Get the XML configuration of the snapshot. */ @@ -13109,6 +13149,8 @@ static const vshCmdOptDef opts_snapshot_list[] = { N_("list only snapshots that have metadata that would prevent undefine")}, {"tree", VSH_OT_BOOL, 0, N_("list snapshots in a tree")}, {"from", VSH_OT_DATA, 0, N_("limit list to children of given snapshot")}, + {"current", VSH_OT_BOOL, 0, + N_("limit list to children of current snapshot")}, {"descendants", VSH_OT_BOOL, 0, N_("with --from, list all descendants")}, {NULL, 0, 0, NULL} }; @@ -13142,10 +13184,17 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) int start_index = -1; bool descendants = false;

- if (vshCommandOptString(cmd, "from", &from) < 0) { - vshError(ctl, _("invalid from argument '%s'"), from); + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + dom = vshCommandOptDomain(ctl, cmd, NULL); + if (dom == NULL) + goto cleanup; + + if ((vshCommandOptBool(cmd, "from") || + vshCommandOptBool(cmd, "current")) && + vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from) < 0) goto cleanup; - }

if (vshCommandOptBool(cmd, "parent")) { if (vshCommandOptBool(cmd, "roots")) { @@ -13176,18 +13225,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; }

- if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; - - dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; - if (from) { descendants = vshCommandOptBool(cmd, "descendants"); - start = virDomainSnapshotLookupByName(dom, from, 0); - if (!start) - goto cleanup; if (descendants || tree) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } @@ -13519,7 +13558,8 @@ static const vshCmdInfo info_snapshot_parent[] = {

static const vshCmdOptDef opts_snapshot_parent[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("find parent of snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("find parent of current snapshot")}, {NULL, 0, 0, NULL} };

@@ -13539,11 +13579,8 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;

- if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) - goto cleanup; - - snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", true, dom, + &snapshot, &name) < 0) goto cleanup;

if (vshGetSnapshotParent(ctl, snapshot, &parent) < 0) @@ -13578,7 +13615,8 @@ static const vshCmdInfo info_snapshot_revert[] = {

static const vshCmdOptDef opts_snapshot_revert[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("revert to current snapshot")}, {"running", VSH_OT_BOOL, 0, N_("after reverting, change state to running")}, {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")}, {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")}, @@ -13614,11 +13652,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;

- if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) - goto cleanup; - - snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", true, dom, + &snapshot, &name) < 0) goto cleanup;

result = virDomainRevertToSnapshot(snapshot, flags); @@ -13654,7 +13689,8 @@ static const vshCmdInfo info_snapshot_delete[] = {

static const vshCmdOptDef opts_snapshot_delete[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"snapshotname", VSH_OT_DATA, 0, N_("snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("delete current snapshot")}, {"children", VSH_OT_BOOL, 0, N_("delete snapshot and all children")}, {"children-only", VSH_OT_BOOL, 0, N_("delete children but not snapshot")}, {"metadata", VSH_OT_BOOL, 0, @@ -13678,7 +13714,8 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;

- if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) + if (vshLookupSnapshot(ctl, cmd, "snapshotname", true, dom, + &snapshot, &name) < 0) goto cleanup;

if (vshCommandOptBool(cmd, "children")) @@ -13688,10 +13725,6 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "metadata")) flags |= VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY;

- snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) - goto cleanup; - /* XXX If we wanted, we could emulate DELETE_CHILDREN_ONLY even on * older servers that reject the flag, by manually computing the * list of descendants. But that's a lot of code to maintain. */ diff --git a/tools/virsh.pod b/tools/virsh.pod index dd60a64..e1a9b86 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1951,11 +1951,13 @@ the XML. With I<snapshotname>, this is a request to make the existing named snapshot become the current snapshot, without reverting the domain.

-=item B<snapshot-edit> I<domain> I<snapshotname> [I<--current>] +=item B<snapshot-edit> I<domain> [I<snapshotname>] [I<--current>]

Edit the XML configuration file for I<snapshotname> of a domain. If -I<--current> is specified, also force the edited snapshot to become -the current snapshot. +both I<snapshotname> and I<--current> are specified, also force the +edited snapshot to become the current snapshot. If I<snapshotname> +is omitted, then I<--current> must be supplied, to edit the current +snapshot.

This is equivalent to:

@@ -1969,7 +1971,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>.

=item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] -[I<--metadata>] [[I<--from>] B<snapshot> [I<--descendants>]] +[I<--metadata>] [{[I<--from>] B<snapshot> | I<--current>} [I<--descendants>]]

List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -1981,7 +1983,8 @@ If I<--tree> is specified, the output will be in a tree format, listing just snapshot names. These three options are mutually exclusive.

If I<--from> is provided, filter the list to snapshots which are -children of the given B<snapshot>. When used in isolation or with +children of the given B<snapshot>; or if I<--current> is provided, +start at the current snapshot. When used in isolation or with I<--parent>, the list is limited to direct children unless I<--descendants> is also present. When used with I<--tree>, the use of I<--descendants> is implied. This option is not compatible @@ -1996,15 +1999,18 @@ a transient domain.

Output the snapshot XML for the domain's snapshot named I<snapshot>. Using I<--security-info> will also include security sensitive information. +Use B<snapshot-current> to easily access the XML of the current snapshot.

-=item B<snapshot-parent> I<domain> I<snapshot> +=item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>}

-Output the name of the parent snapshot for the given I<snapshot>, if any. +Output the name of the parent snapshot, if any, for the given +I<snapshot>, or for the current snapshot with I<--current>.

-=item B<snapshot-revert> I<domain> I<snapshot> [{I<--running> | I<--paused>}] -[I<--force>] +=item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} +[{I<--running> | I<--paused>}] [I<--force>]

-Revert the given domain to the snapshot specified by I<snapshot>. Be aware +Revert the given domain to the snapshot specified by I<snapshot>, or to +the current snapshot with I<--current>. Be aware that this is a destructive action; any changes in the domain since the last snapshot was taken will be lost. Also note that the state of the domain after snapshot-revert is complete will be the state of the domain at the time @@ -2034,10 +2040,11 @@ snapshot that uses a provably incompatible configuration, as well as with an inactive snapshot that is combined with the I<--start> or I<--pause> flag.

-=item B<snapshot-delete> I<domain> I<snapshot> [I<--metadata>] +=item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>} [I<--metadata>] [{I<--children> | I<--children-only>}]

-Delete the snapshot for the domain named I<snapshot>. If this snapshot +Delete the snapshot for the domain named I<snapshot>, or the current +snapshot with I<--current>. If this snapshot has child snapshots, changes from this snapshot will be merged into the children. If I<--children> is passed, then delete this snapshot and any children of this snapshot. If I<--children-only> is passed, then delete

ACK, an useful addition ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/