[libvirt] [PATCH 0/6] refactor 'virsh snapshot-list'
I'm in the middle of writing a patch series for virDomainListAllSnapshots; this series is preliminary to that, and I'd like to get the review started now. The end result of this series is a net gain in lines of code, but a much nicer framework for changing just one point in the code to use new APIs and having everything else continue to work as before. Eric Blake (6): buf: support peeking at string contents virsh: remove limits on tree listing virsh: make tree listing more flexible snapshot: virsh indentation cleanup snapshot: new virsh function factored from snapshot-list snapshot: use new virsh function for snapshot-list src/libvirt_private.syms | 1 + src/util/buf.c | 22 +- src/util/buf.h | 1 + tests/virbuftest.c | 8 + tools/virsh.c | 735 +++++++++++++++++++++++++++------------------- 5 files changed, 466 insertions(+), 301 deletions(-) -- 1.7.10.2
Right now, the only way to get at the contents of a virBuffer is to destroy it. But there are cases in my upcoming patches where peeking at the contents makes life easier. I suppose this does open up the potential for bad code to dereference a stale pointer, by disregarding the docs that the return value is invalid on the next virBuf operation, but such is life. * src/util/buf.h (virBufferCurrentContent): New declaration. * src/util/buf.c (virBufferCurrentContent): Implement it. * src/libvirt_private.syms (buf.h): Export it. * tests/virbuftest.c (testBufAutoIndent): Test it. --- src/libvirt_private.syms | 1 + src/util/buf.c | 22 +++++++++++++++++++++- src/util/buf.h | 1 + tests/virbuftest.c | 8 ++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf2186..ef8047d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -20,6 +20,7 @@ virBufferAddChar; virBufferAdjustIndent; virBufferAsprintf; virBufferContentAndReset; +virBufferCurrentContent; virBufferError; virBufferEscape; virBufferEscapeSexpr; diff --git a/src/util/buf.c b/src/util/buf.c index 630e4c9..6c7c501 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -175,12 +175,32 @@ virBufferAddChar(virBufferPtr buf, char c) } /** + * virBufferCurrentContent: + * @buf: Buffer + * + * Get the current content from the buffer. The content is only valid + * until the next operation on @buf, and an empty string is returned if + * no content is present yet. + * + * Returns the buffer content or NULL in case of error. + */ +const char * +virBufferCurrentContent(virBufferPtr buf) +{ + if (!buf || buf->error) + return NULL; + return buf->use ? buf->content : ""; +} + +/** * virBufferContentAndReset: * @buf: Buffer * * Get the content from the buffer and free (only) the buffer structure. * The caller owns the returned string & should free it when no longer - * required. The buffer object is reset to its initial state. + * required. The buffer object is reset to its initial state. This + * interface intentionally returns NULL instead of an empty string if + * there is no content. * * Returns the buffer content or NULL in case of error. */ diff --git a/src/util/buf.h b/src/util/buf.h index a8e2eb5..2750b17 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -37,6 +37,7 @@ struct _virBuffer { }; # endif +const char *virBufferCurrentContent(virBufferPtr buf); char *virBufferContentAndReset(virBufferPtr buf); void virBufferFreeAndReset(virBufferPtr buf); int virBufferError(const virBufferPtr buf); diff --git a/tests/virbuftest.c b/tests/virbuftest.c index cd02db1..7af679c 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -73,6 +73,10 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) ret = -1; } virBufferAdjustIndent(buf, 3); + if (STRNEQ_NULLABLE(virBufferCurrentContent(buf), "")) { + TEST_ERROR("Wrong content"); + ret = -1; + } if (virBufferGetIndent(buf, false) != 3 || virBufferGetIndent(buf, true) != 3 || virBufferError(buf)) { @@ -102,6 +106,10 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) } virBufferAdjustIndent(buf, 2); virBufferAddLit(buf, "1"); + if (STRNEQ_NULLABLE(virBufferCurrentContent(buf), " 1")) { + TEST_ERROR("Wrong content"); + ret = -1; + } if (virBufferGetIndent(buf, false) != 2 || virBufferGetIndent(buf, true) != 0) { TEST_ERROR("Wrong indentation"); -- 1.7.10.2
On 06/09/12 06:34, Eric Blake wrote:
Right now, the only way to get at the contents of a virBuffer is to destroy it. But there are cases in my upcoming patches where peeking at the contents makes life easier. I suppose this does open up the potential for bad code to dereference a stale pointer, by disregarding the docs that the return value is invalid on the next virBuf operation, but such is life.
* src/util/buf.h (virBufferCurrentContent): New declaration. * src/util/buf.c (virBufferCurrentContent): Implement it. * src/libvirt_private.syms (buf.h): Export it. * tests/virbuftest.c (testBufAutoIndent): Test it. ---
ACK. Might be useful sometimes. I looked for something like this once. Peter
On 06/11/2012 06:05 AM, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
Right now, the only way to get at the contents of a virBuffer is to destroy it. But there are cases in my upcoming patches where peeking at the contents makes life easier. I suppose this does open up the potential for bad code to dereference a stale pointer, by disregarding the docs that the return value is invalid on the next virBuf operation, but such is life.
* src/util/buf.h (virBufferCurrentContent): New declaration. * src/util/buf.c (virBufferCurrentContent): Implement it. * src/libvirt_private.syms (buf.h): Export it. * tests/virbuftest.c (testBufAutoIndent): Test it. ---
ACK. Might be useful sometimes. I looked for something like this once.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
I am not a fan of fixed-width buffers. All it takes is a linear chain of more than 100 snapshots to mess up 'virsh snapshot-list --tree'. Now that virBuffer is more powerful, we might as well exploit its power. * tools/virsh.c (cmdNodeListDevicesPrint): Simplify to use a virBuffer instead of fixed-width prefix, factor guts, and rename... (vshTreePrint, vshTreePrintInternal): ...along with new helper. (cmdNodeListDevices, cmdSnapshotList): Update callers. --- tools/virsh.c | 172 ++++++++++++++++++++++++++------------------------------- 1 file changed, 78 insertions(+), 94 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b28dc49..0d67f4b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13165,60 +13165,34 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; } -/* - * "nodedev-list" command - */ -static const vshCmdInfo info_node_list_devices[] = { - {"help", N_("enumerate devices on this host")}, - {"desc", ""}, - {NULL, NULL} -}; - -static const vshCmdOptDef opts_node_list_devices[] = { - {"tree", VSH_OT_BOOL, 0, N_("list devices in a tree")}, - {"cap", VSH_OT_STRING, VSH_OFLAG_NONE, N_("capability name")}, - {NULL, 0, 0, NULL} -}; - -#define MAX_DEPTH 100 -#define INDENT_SIZE 4 -#define INDENT_BUFLEN ((MAX_DEPTH * INDENT_SIZE) + 1) - -static void -cmdNodeListDevicesPrint(vshControl *ctl, - char **devices, - char **parents, - int num_devices, - int devid, - int lastdev, - unsigned int depth, - unsigned int indentIdx, - char *indentBuf) +/* Tree listing helpers. */ +static int +vshTreePrintInternal(vshControl *ctl, + char **devices, + char **parents, + int num_devices, + int devid, + int lastdev, + bool root, + virBufferPtr indent) { int i; int nextlastdev = -1; + int ret = -1; - /* Prepare indent for this device, but not if at root */ - if (depth && depth < MAX_DEPTH) { - indentBuf[indentIdx] = '+'; - indentBuf[indentIdx+1] = '-'; - indentBuf[indentIdx+2] = ' '; - indentBuf[indentIdx+3] = '\0'; - } - - /* Print this device */ - vshPrint(ctl, "%s", indentBuf); - vshPrint(ctl, "%s\n", devices[devid]); + if (virBufferError(indent)) + goto cleanup; + /* Print this device, with indent if not at root */ + vshPrint(ctl, "%s%s%s\n", virBufferCurrentContent(indent), + root ? "" : "+- ", devices[devid]); /* Update indent to show '|' or ' ' for child devices */ - if (depth && depth < MAX_DEPTH) { - if (devid == lastdev) - indentBuf[indentIdx] = ' '; - else - indentBuf[indentIdx] = '|'; - indentBuf[indentIdx+1] = ' '; - indentIdx+=2; + if (!root) { + virBufferAddChar(indent, devid == lastdev ? ' ' : '|'); + virBufferAddChar(indent, ' '); + if (virBufferError(indent)) + goto cleanup; } /* Determine the index of the last child device */ @@ -13230,43 +13204,70 @@ cmdNodeListDevicesPrint(vshControl *ctl, } /* If there is a child device, then print another blank line */ - if (nextlastdev != -1) { - vshPrint(ctl, "%s", indentBuf); - vshPrint(ctl, " |\n"); - } + if (nextlastdev != -1) + vshPrint(ctl, "%s |\n", virBufferCurrentContent(indent)); /* Finally print all children */ - if (depth < MAX_DEPTH) - indentBuf[indentIdx] = ' '; + virBufferAddLit(indent, " "); for (i = 0 ; i < num_devices ; i++) { - if (depth < MAX_DEPTH) { - indentBuf[indentIdx] = ' '; - indentBuf[indentIdx+1] = ' '; - } - if (parents[i] && - STREQ(parents[i], devices[devid])) - cmdNodeListDevicesPrint(ctl, devices, parents, - num_devices, i, nextlastdev, - depth + 1, indentIdx + 2, indentBuf); - if (depth < MAX_DEPTH) - indentBuf[indentIdx] = '\0'; + if (parents[i] && STREQ(parents[i], devices[devid]) && + vshTreePrintInternal(ctl, devices, parents, + num_devices, i, nextlastdev, + false, indent) < 0) + goto cleanup; } + virBufferTrim(indent, " ", -1); /* If there was no child device, and we're the last in * a list of devices, then print another blank line */ - if (nextlastdev == -1 && devid == lastdev) { - vshPrint(ctl, "%s", indentBuf); - vshPrint(ctl, "\n"); - } + if (nextlastdev == -1 && devid == lastdev) + vshPrint(ctl, "%s\n", virBufferCurrentContent(indent)); + + if (!root) + virBufferTrim(indent, NULL, 2); + ret = 0; +cleanup: + return ret; +} + +static int +vshTreePrint(vshControl *ctl, char **devices, char **parents, + int num_devices, int devid) +{ + int ret; + virBuffer indent = VIR_BUFFER_INITIALIZER; + + ret = vshTreePrintInternal(ctl, devices, parents, num_devices, + devid, devid, true, &indent); + if (ret < 0) + vshError(ctl, "%s", _("Failed to complete tree listing")); + virBufferFreeAndReset(&indent); + return ret; } +/* + * "nodedev-list" command + */ +static const vshCmdInfo info_node_list_devices[] = { + {"help", N_("enumerate devices on this host")}, + {"desc", ""}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_node_list_devices[] = { + {"tree", VSH_OT_BOOL, 0, N_("list devices in a tree")}, + {"cap", VSH_OT_STRING, VSH_OFLAG_NONE, N_("capability name")}, + {NULL, 0, 0, NULL} +}; + static bool -cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { const char *cap = NULL; char **devices; int num_devices, i; bool tree = vshCommandOptBool(cmd, "tree"); + bool ret = true; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -13292,8 +13293,8 @@ cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } qsort(&devices[0], num_devices, sizeof(char*), namesorter); if (tree) { - char indentBuf[INDENT_BUFLEN]; char **parents = vshMalloc(ctl, sizeof(char *) * num_devices); + for (i = 0; i < num_devices; i++) { virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]); if (dev && STRNEQ(devices[i], "computer")) { @@ -13305,17 +13306,9 @@ cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virNodeDeviceFree(dev); } for (i = 0 ; i < num_devices ; i++) { - memset(indentBuf, '\0', sizeof(indentBuf)); - if (parents[i] == NULL) - cmdNodeListDevicesPrint(ctl, - devices, - parents, - num_devices, - i, - i, - 0, - 0, - indentBuf); + if (parents[i] == NULL && + vshTreePrint(ctl, devices, parents, num_devices, i) < 0) + ret = false; } for (i = 0 ; i < num_devices ; i++) { VIR_FREE(devices[i]); @@ -13329,7 +13322,7 @@ cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } } VIR_FREE(devices); - return true; + return ret; } /* @@ -16763,20 +16756,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } } if (tree) { - char indentBuf[INDENT_BUFLEN]; for (i = 0 ; i < actual ; i++) { - memset(indentBuf, '\0', sizeof(indentBuf)); if ((from && ctl->useSnapshotOld) ? STREQ(names[i], from) : - !parents[i]) - cmdNodeListDevicesPrint(ctl, - names, - parents, - actual, - i, - i, - 0, - 0, - indentBuf); + !parents[i] && + vshTreePrint(ctl, names, parents, actual, i) < 0) + goto cleanup; } ret = true; -- 1.7.10.2
On 06/09/12 06:34, Eric Blake wrote:
I am not a fan of fixed-width buffers. All it takes is a linear chain of more than 100 snapshots to mess up 'virsh snapshot-list --tree'. Now that virBuffer is more powerful, we might as well exploit its power.
* tools/virsh.c (cmdNodeListDevicesPrint): Simplify to use a virBuffer instead of fixed-width prefix, factor guts, and rename... (vshTreePrint, vshTreePrintInternal): ...along with new helper. (cmdNodeListDevices, cmdSnapshotList): Update callers. ---
ACK, great improvement in code understandability. Peter
On 06/11/2012 06:31 AM, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
I am not a fan of fixed-width buffers. All it takes is a linear chain of more than 100 snapshots to mess up 'virsh snapshot-list --tree'. Now that virBuffer is more powerful, we might as well exploit its power.
* tools/virsh.c (cmdNodeListDevicesPrint): Simplify to use a virBuffer instead of fixed-width prefix, factor guts, and rename... (vshTreePrint, vshTreePrintInternal): ...along with new helper. (cmdNodeListDevices, cmdSnapshotList): Update callers. ---
ACK, great improvement in code understandability.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Requiring the user to pass in parallel arrays of names and parents is annoying; it means that you can't qsort one of the arrays without invalidating the ordering of the other. By refactoring this function to use callbacks, we isolate the layout to be independent of the printing, and a future patch can exploit that to improve layout. * tools/virsh.c (vshTreePrintInternal): Use callbacks rather than requiring a char** array. (vshTreeArrayLookup): New helper function. (vshTreePrint, cmdNodeListDevices, cmdSnapshotList): Update callers. --- tools/virsh.c | 50 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 0d67f4b..85e618c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13166,10 +13166,15 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* Tree listing helpers. */ + +/* Given an index, return either the name of that device (non-NULL) or + * of its parent (NULL if a root). */ +typedef const char * (*vshTreeLookup)(int devid, bool parent, void *opaque); + static int vshTreePrintInternal(vshControl *ctl, - char **devices, - char **parents, + vshTreeLookup lookup, + void *opaque, int num_devices, int devid, int lastdev, @@ -13179,13 +13184,14 @@ vshTreePrintInternal(vshControl *ctl, int i; int nextlastdev = -1; int ret = -1; + const char *dev = (lookup)(devid, false, opaque); if (virBufferError(indent)) goto cleanup; /* Print this device, with indent if not at root */ vshPrint(ctl, "%s%s%s\n", virBufferCurrentContent(indent), - root ? "" : "+- ", devices[devid]); + root ? "" : "+- ", dev); /* Update indent to show '|' or ' ' for child devices */ if (!root) { @@ -13197,10 +13203,10 @@ vshTreePrintInternal(vshControl *ctl, /* Determine the index of the last child device */ for (i = 0 ; i < num_devices ; i++) { - if (parents[i] && - STREQ(parents[i], devices[devid])) { + const char *parent = (lookup)(i, true, opaque); + + if (parent && STREQ(parent, dev)) nextlastdev = i; - } } /* If there is a child device, then print another blank line */ @@ -13210,8 +13216,10 @@ vshTreePrintInternal(vshControl *ctl, /* Finally print all children */ virBufferAddLit(indent, " "); for (i = 0 ; i < num_devices ; i++) { - if (parents[i] && STREQ(parents[i], devices[devid]) && - vshTreePrintInternal(ctl, devices, parents, + const char *parent = (lookup)(i, true, opaque); + + if (parent && STREQ(parent, dev) && + vshTreePrintInternal(ctl, lookup, opaque, num_devices, i, nextlastdev, false, indent) < 0) goto cleanup; @@ -13231,13 +13239,13 @@ cleanup: } static int -vshTreePrint(vshControl *ctl, char **devices, char **parents, +vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, int num_devices, int devid) { int ret; virBuffer indent = VIR_BUFFER_INITIALIZER; - ret = vshTreePrintInternal(ctl, devices, parents, num_devices, + ret = vshTreePrintInternal(ctl, lookup, opaque, num_devices, devid, devid, true, &indent); if (ret < 0) vshError(ctl, "%s", _("Failed to complete tree listing")); @@ -13245,6 +13253,20 @@ vshTreePrint(vshControl *ctl, char **devices, char **parents, return ret; } +struct vshTreeArray { + char **names; + char **parents; +}; + +static const char * +vshTreeArrayLookup(int devid, bool parent, void *opaque) +{ + struct vshTreeArray *arrays = opaque; + if (parent) + return arrays->parents[devid]; + return arrays->names[devid]; +} + /* * "nodedev-list" command */ @@ -13294,6 +13316,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) qsort(&devices[0], num_devices, sizeof(char*), namesorter); if (tree) { char **parents = vshMalloc(ctl, sizeof(char *) * num_devices); + struct vshTreeArray arrays = { devices, parents }; for (i = 0; i < num_devices; i++) { virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]); @@ -13307,7 +13330,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } for (i = 0 ; i < num_devices ; i++) { if (parents[i] == NULL && - vshTreePrint(ctl, devices, parents, num_devices, i) < 0) + vshTreePrint(ctl, vshTreeArrayLookup, &arrays, num_devices, + i) < 0) ret = false; } for (i = 0 ; i < num_devices ; i++) { @@ -16756,10 +16780,12 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } } if (tree) { + struct vshTreeArray arrays = { names, parents }; + for (i = 0 ; i < actual ; i++) { if ((from && ctl->useSnapshotOld) ? STREQ(names[i], from) : !parents[i] && - vshTreePrint(ctl, names, parents, actual, i) < 0) + vshTreePrint(ctl, vshTreeArrayLookup, &arrays, actual, i) < 0) goto cleanup; } -- 1.7.10.2
On 06/09/12 06:34, Eric Blake wrote:
Requiring the user to pass in parallel arrays of names and parents is annoying; it means that you can't qsort one of the arrays without invalidating the ordering of the other. By refactoring this function to use callbacks, we isolate the layout to be independent of the printing, and a future patch can exploit that to improve layout.
* tools/virsh.c (vshTreePrintInternal): Use callbacks rather than requiring a char** array. (vshTreeArrayLookup): New helper function. (vshTreePrint, cmdNodeListDevices, cmdSnapshotList): Update callers. --- tools/virsh.c | 50 ++++++++++++++++++++++++++++++++++++++------------ @@ -13245,6 +13253,20 @@ vshTreePrint(vshControl *ctl, char **devices, char **parents, return ret; }
+struct vshTreeArray { + char **names; + char **parents; +}; + +static const char * +vshTreeArrayLookup(int devid, bool parent, void *opaque) +{ + struct vshTreeArray *arrays = opaque; + if (parent) + return arrays->parents[devid]; + return arrays->names[devid]; +} +
This is used just for listing node devices IIUC. Wouln't it be better to explicitly name the helper and struct that it's meant for node devices? vshNodeDeviceTree ... ?
/* * "nodedev-list" command */
ACK. The name doesn't matter that much, making the tree printer more universal does. Peter
On 06/11/2012 07:13 AM, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
Requiring the user to pass in parallel arrays of names and parents is annoying; it means that you can't qsort one of the arrays without invalidating the ordering of the other. By refactoring this function to use callbacks, we isolate the layout to be independent of the printing, and a future patch can exploit that to improve layout.
* tools/virsh.c (vshTreePrintInternal): Use callbacks rather than requiring a char** array. (vshTreeArrayLookup): New helper function. (vshTreePrint, cmdNodeListDevices, cmdSnapshotList): Update callers.
+static const char * +vshTreeArrayLookup(int devid, bool parent, void *opaque) +{ + struct vshTreeArray *arrays = opaque; + if (parent) + return arrays->parents[devid]; + return arrays->names[devid]; +} +
This is used just for listing node devices IIUC. Wouln't it be better to explicitly name the helper and struct that it's meant for node devices? vshNodeDeviceTree ... ?
In _this_ patch, it's used by both node devices and snapshots. Later, in patch 6/6, I switch snapshots over to using a different callback. Maybe I should rename this function as part of that later split into two different callbacks, but for this patch, since it really is used twice, I think it makes sense as-is.
ACK. The name doesn't matter that much, making the tree printer more universal does.
Peter
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/11/2012 07:43 AM, Eric Blake wrote:
On 06/11/2012 07:13 AM, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
Requiring the user to pass in parallel arrays of names and parents is annoying; it means that you can't qsort one of the arrays without invalidating the ordering of the other. By refactoring this function to use callbacks, we isolate the layout to be independent of the printing, and a future patch can exploit that to improve layout.
+static const char * +vshTreeArrayLookup(int devid, bool parent, void *opaque)
This is used just for listing node devices IIUC. Wouln't it be better to explicitly name the helper and struct that it's meant for node devices? vshNodeDeviceTree ... ?
In _this_ patch, it's used by both node devices and snapshots. Later, in patch 6/6, I switch snapshots over to using a different callback. Maybe I should rename this function as part of that later split into two different callbacks, but for this patch, since it really is used twice, I think it makes sense as-is.
ACK. The name doesn't matter that much, making the tree printer more universal does.
I've pushed this now, but will remain open to the idea of reworking 6/6 to adjust naming. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
No semantic change; this will make it easier to refactor code. * tools/virsh.c (cmdSnapshotList): Drop level of indentation, and rename a variable. --- tools/virsh.c | 238 ++++++++++++++++++++++++++++----------------------------- 1 file changed, 118 insertions(+), 120 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 85e618c..de7b282 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16600,10 +16600,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; int parent_filter = 0; /* -1 for roots filtering, 0 for no parent information needed, 1 for parent column */ - int numsnaps; char **names = NULL; char **parents = NULL; - int actual = 0; + int count = 0; int i; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; @@ -16677,9 +16676,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (descendants || tree) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } - numsnaps = ctl->useSnapshotOld ? -1 : + count = ctl->useSnapshotOld ? -1 : virDomainSnapshotNumChildren(start, flags); - if (numsnaps < 0) { + if (count < 0) { if (ctl->useSnapshotOld || last_error->code == VIR_ERR_NO_SUPPORT) { /* We can emulate --from. */ @@ -16688,27 +16687,27 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) last_error = NULL; ctl->useSnapshotOld = true; flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - numsnaps = virDomainSnapshotNum(dom, flags); + count = virDomainSnapshotNum(dom, flags); } } else if (tree) { - numsnaps++; + count++; } } else { - numsnaps = virDomainSnapshotNum(dom, flags); + count = virDomainSnapshotNum(dom, flags); /* Fall back to simulation if --roots was unsupported. */ /* XXX can we also emulate --leaves? */ - if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && + if (count < 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); + count = virDomainSnapshotNum(dom, flags); } } - if (numsnaps < 0) { + if (count < 0) { if (!last_error) vshError(ctl, _("missing support")); goto cleanup; @@ -16726,41 +16725,40 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) "------------------------------------------------------------\n"); } - if (!numsnaps) { + if (!count) { ret = true; goto cleanup; } - if (VIR_ALLOC_N(names, numsnaps) < 0) + if (VIR_ALLOC_N(names, count) < 0) goto cleanup; 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) { - if (numsnaps) - actual = virDomainSnapshotListChildrenNames(start, names + 1, - numsnaps - 1, - flags); - if (actual >= 0) { - actual++; + if (count) + count = virDomainSnapshotListChildrenNames(start, names + 1, + count - 1, flags); + if (count >= 0) { + count++; names[0] = vshStrdup(ctl, from); } } else { - actual = virDomainSnapshotListChildrenNames(start, names, - numsnaps, flags); + count = virDomainSnapshotListChildrenNames(start, names, + count, flags); } } else { - actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); + count = virDomainSnapshotListNames(dom, names, count, flags); } - if (actual < 0) + if (count < 0) goto cleanup; - qsort(&names[0], actual, sizeof(char*), namesorter); + qsort(&names[0], count, sizeof(char*), namesorter); if (tree || (from && ctl->useSnapshotOld)) { - parents = vshCalloc(ctl, sizeof(char *), actual); - for (i = (from && !ctl->useSnapshotOld); i < actual; i++) { + parents = vshCalloc(ctl, sizeof(char *), count); + for (i = (from && !ctl->useSnapshotOld); i < count; i++) { if (from && ctl->useSnapshotOld && STREQ(names[i], from)) { start_index = i; continue; @@ -16782,123 +16780,123 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { struct vshTreeArray arrays = { names, parents }; - for (i = 0 ; i < actual ; i++) { + for (i = 0 ; i < count ; i++) { if ((from && ctl->useSnapshotOld) ? STREQ(names[i], from) : !parents[i] && - vshTreePrint(ctl, vshTreeArrayLookup, &arrays, actual, i) < 0) + vshTreePrint(ctl, vshTreeArrayLookup, &arrays, count, i) < 0) goto cleanup; } ret = true; goto cleanup; - } else { - if (ctl->useSnapshotOld && descendants) { - bool changed = false; - - /* Make multiple passes over the list - first pass NULLs - * out all roots except start, remaining passes NULL out - * any entry whose parent is not still in list. Also, we - * NULL out parent when name is known to be in list. - * Sorry, this is O(n^3) - hope your hierarchy isn't huge. */ - if (start_index < 0) { - vshError(ctl, _("snapshot %s disappeared from list"), from); - goto cleanup; - } - for (i = 0; i < actual; i++) { - if (i == start_index) - continue; - if (!parents[i]) { - VIR_FREE(names[i]); - } else if (STREQ(parents[i], from)) { - VIR_FREE(parents[i]); - changed = true; - } - } - if (!changed) { - ret = true; - goto cleanup; + } + + if (ctl->useSnapshotOld && descendants) { + bool changed = false; + + /* Make multiple passes over the list - first pass NULLs out + * all roots except start, remaining passes NULL out any entry + * whose parent is not still in list. Also, we NULL out + * parent when name is known to be in list. Sorry, this is + * O(n^3) - hope your hierarchy isn't huge. */ + if (start_index < 0) { + vshError(ctl, _("snapshot %s disappeared from list"), from); + goto cleanup; + } + for (i = 0; i < count; i++) { + if (i == start_index) + continue; + if (!parents[i]) { + VIR_FREE(names[i]); + } else if (STREQ(parents[i], from)) { + VIR_FREE(parents[i]); + changed = true; } - while (changed) { - changed = false; - for (i = 0; i < actual; i++) { - bool found = false; - int j; + } + if (!changed) { + ret = true; + goto cleanup; + } + while (changed) { + changed = false; + for (i = 0; i < count; i++) { + bool found = false; + int j; - if (!names[i] || !parents[i]) + if (!names[i] || !parents[i]) + continue; + for (j = 0; j < count; j++) { + if (!names[j] || i == j) continue; - for (j = 0; j < actual; j++) { - if (!names[j] || i == j) - continue; - if (STREQ(parents[i], names[j])) { - found = true; - if (!parents[j]) - VIR_FREE(parents[i]); - break; - } - } - if (!found) { - changed = true; - VIR_FREE(names[i]); + if (STREQ(parents[i], names[j])) { + found = true; + if (!parents[j]) + VIR_FREE(parents[i]); + break; } } + if (!found) { + changed = true; + VIR_FREE(names[i]); + } } - VIR_FREE(names[start_index]); } + VIR_FREE(names[start_index]); + } - for (i = 0; i < actual; i++) { - if (from && ctl->useSnapshotOld && - (descendants ? !names[i] : STRNEQ_NULLABLE(parents[i], from))) - continue; - - /* free up memory from previous iterations of the loop */ - VIR_FREE(parent); - VIR_FREE(state); - if (snapshot) - virDomainSnapshotFree(snapshot); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - VIR_FREE(doc); + for (i = 0; i < count; i++) { + if (from && ctl->useSnapshotOld && + (descendants ? !names[i] : STRNEQ_NULLABLE(parents[i], from))) + continue; - snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); - if (snapshot == NULL) - continue; + /* free up memory from previous iterations of the loop */ + VIR_FREE(parent); + VIR_FREE(state); + if (snapshot) + virDomainSnapshotFree(snapshot); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + VIR_FREE(doc); - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) - continue; + snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); + if (snapshot == NULL) + continue; - xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt); - if (!xml) - continue; + doc = virDomainSnapshotGetXMLDesc(snapshot, 0); + if (!doc) + continue; - if (parent_filter) { - parent = virXPathString("string(/domainsnapshot/parent/name)", - ctxt); - if (!parent && parent_filter < 0) - continue; - } + xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt); + if (!xml) + continue; - state = virXPathString("string(/domainsnapshot/state)", ctxt); - if (state == NULL) - continue; - if (virXPathLongLong("string(/domainsnapshot/creationTime)", ctxt, - &creation_longlong) < 0) + if (parent_filter) { + parent = virXPathString("string(/domainsnapshot/parent/name)", + ctxt); + if (!parent && parent_filter < 0) continue; - creation_time_t = creation_longlong; - if (creation_time_t != creation_longlong) { - vshError(ctl, "%s", _("time_t overflow")); - continue; - } - localtime_r(&creation_time_t, &time_info); - strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", - &time_info); + } - if (parent) - vshPrint(ctl, " %-20s %-25s %-15s %s\n", - names[i], timestr, state, parent); - else - vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); + state = virXPathString("string(/domainsnapshot/state)", ctxt); + if (state == NULL) + continue; + if (virXPathLongLong("string(/domainsnapshot/creationTime)", ctxt, + &creation_longlong) < 0) + continue; + creation_time_t = creation_longlong; + if (creation_time_t != creation_longlong) { + vshError(ctl, "%s", _("time_t overflow")); + continue; } + localtime_r(&creation_time_t, &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", + &time_info); + + if (parent) + vshPrint(ctl, " %-20s %-25s %-15s %s\n", + names[i], timestr, state, parent); + else + vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); } ret = true; @@ -16914,7 +16912,7 @@ cleanup: xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - for (i = 0; i < actual; i++) { + for (i = 0; i < count; i++) { VIR_FREE(names[i]); if (parents) VIR_FREE(parents[i]); -- 1.7.10.2
On 06/09/12 06:34, Eric Blake wrote:
No semantic change; this will make it easier to refactor code.
* tools/virsh.c (cmdSnapshotList): Drop level of indentation, and rename a variable. ---
git show -b is the magic formula to help with indentation patch review :) ACK Peter
On 06/11/2012 07:52 AM, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
No semantic change; this will make it easier to refactor code.
* tools/virsh.c (cmdSnapshotList): Drop level of indentation, and rename a variable. ---
git show -b is the magic formula to help with indentation patch review :)
ACK
Thanks; pushed. When reviewing 5/6 myself before posting, I found it easiest to compare the new code in one window side-by-side with the existing cmdSnapshotList code in another window. Now that the indentation and naming is similar, the differences boil down to a different representation (single struct instead of parallel arrays) and deleting entries from that array. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch copies just the fallback code out of cmdSnapshotList, and keeps the snapshot objects around rather than just their name for easier manipulation. It looks forward to a future patch when we add a way to list all snapshot objects at once, and the next patch will simplify cmdSnapshotList to take advantage of this factorization. * tools/virsh.c (vshSnapshotList, vshSnapshotListFree): New functions. --- tools/virsh.c | 272 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 272 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index de7b282..62546b2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16568,6 +16568,278 @@ cleanup: return ret; } +/* Helpers for collecting a list of snapshots. */ +struct vshSnap { + virDomainSnapshotPtr snap; + char *parent; +}; +struct vshSnapshotList { + struct vshSnap *snaps; + int nsnaps; +}; +typedef struct vshSnapshotList *vshSnapshotListPtr; + +static void +vshSnapshotListFree(vshSnapshotListPtr snaplist) +{ + int i; + + if (!snaplist) + return; + if (snaplist->snaps) { + for (i = 0; i < snaplist->nsnaps; i++) { + if (snaplist->snaps[i].snap) + virDomainSnapshotFree(snaplist->snaps[i].snap); + VIR_FREE(snaplist->snaps[i].parent); + } + VIR_FREE(snaplist->snaps); + } + VIR_FREE(snaplist); +} + +static int +vshSnapSorter(const void *a, const void *b) +{ + const struct vshSnap *sa = a; + const struct vshSnap *sb = b; + + if (sa->snap && !sb->snap) + return 1; + if (!sa->snap && sb->snap) + return -1; + if (!sa->snap) + return 0; + + /* User visible sort, so we want locale-specific case comparison. */ + return strcasecmp(virDomainSnapshotGetName(sa->snap), + virDomainSnapshotGetName(sb->snap)); +} + +/* Compute a list of snapshots from DOM. If FROM is provided, the + * list is limited to descendants of the given snapshot. If FLAGS is + * given, the list is filtered. If TREE is specified, then the + * parents array of SNAPLIST will also be set in a manner usable by + * tree listings (note that parents may also be set when getting + * descendants using older servers). */ +static vshSnapshotListPtr ATTRIBUTE_UNUSED +vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, + virDomainSnapshotPtr from, + unsigned int flags, bool tree) +{ + int i; + char **names = NULL; + int count = 0; + bool descendants = false; + bool roots = false; + vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist)); + vshSnapshotListPtr ret = NULL; + const char *fromname = NULL; + int start_index = -1; + int deleted = 0; + + /* 0.9.13 will be adding a new listing API. */ + + /* This is the interface available in 0.9.12 and earlier, + * including fallbacks to 0.9.4 and earlier. */ + if (from) { + fromname = virDomainSnapshotGetName(from); + if (!fromname) { + vshError(ctl, "%s", _("Could not get snapshot name")); + goto cleanup; + } + descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0; + if (tree) + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + count = ctl->useSnapshotOld ? -1 : + virDomainSnapshotNumChildren(from, flags); + if (count < 0) { + if (ctl->useSnapshotOld || + last_error->code == VIR_ERR_NO_SUPPORT) { + /* We can emulate --from. */ + /* XXX can we also emulate --leaves? */ + virFreeError(last_error); + last_error = NULL; + ctl->useSnapshotOld = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + count = virDomainSnapshotNum(dom, flags); + } + } else if (tree) { + count++; + } + } else { + count = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + /* XXX can we also emulate --leaves? */ + if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + roots = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + count = virDomainSnapshotNum(dom, flags); + } + } + + if (count < 0) { + if (!last_error) + vshError(ctl, _("missing support")); + goto cleanup; + } + + if (!count) + goto success; + + names = vshCalloc(ctl, sizeof(*names), count); + + if (from && !ctl->useSnapshotOld) { + if (tree) { + if (count) + count = virDomainSnapshotListChildrenNames(from, names + 1, + count - 1, flags); + if (count >= 0) { + count++; + names[0] = vshStrdup(ctl, fromname); + } + } else { + count = virDomainSnapshotListChildrenNames(from, names, + count, flags); + } + } else { + count = virDomainSnapshotListNames(dom, names, count, flags); + } + if (count < 0) + goto cleanup; + + snaplist->snaps = vshCalloc(ctl, sizeof(*snaplist->snaps), count); + snaplist->nsnaps = count; + for (i = 0; i < count; i++) { + snaplist->snaps[i].snap = virDomainSnapshotLookupByName(dom, + names[i], 0); + if (!snaplist->snaps[i].snap) + goto cleanup; + } + + if (tree || (from && ctl->useSnapshotOld)) { + for (i = (from && !ctl->useSnapshotOld); i < count; i++) { + if (from && ctl->useSnapshotOld && STREQ(names[i], fromname)) { + start_index = i; + continue; + } + if (vshGetSnapshotParent(ctl, snaplist->snaps[i].snap, + &snaplist->snaps[i].parent) < 0) + goto cleanup; + } + } + if (tree) { + if (from && ctl->useSnapshotOld) { + /* Leave things so that the only entry without a parent is + * 'from'. */ + for (i = 0; i < count; i++) { + if (STREQ(virDomainSnapshotGetName(snaplist->snaps[i].snap), + fromname)) { + VIR_FREE(snaplist->snaps[i].parent); + } else if (!snaplist->snaps[i].parent) { + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + deleted++; + } + } + } + goto success; + } + + if (ctl->useSnapshotOld && descendants) { + bool changed = false; + + /* Make multiple passes over the list - first pass NULLs out + * all roots except start, remaining passes NULL out any entry + * whose parent is not still in list. Also, we NULL out + * parent when name is known to be in list. Sorry, this is + * O(n^3) - hope your hierarchy isn't huge. */ + if (start_index < 0) { + vshError(ctl, _("snapshot %s disappeared from list"), fromname); + goto cleanup; + } + for (i = 0; i < count; i++) { + if (i == start_index) + continue; + if (!snaplist->snaps[i].parent) { + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } else if (STREQ(snaplist->snaps[i].parent, fromname)) { + VIR_FREE(snaplist->snaps[i].parent); + changed = true; + } + } + if (!changed) + goto success; + while (changed) { + changed = false; + for (i = 0; i < count; i++) { + bool found = false; + int j; + + if (!names[i] || !snaplist->snaps[i].parent) + continue; + for (j = 0; j < count; j++) { + if (!names[j] || i == j) + continue; + if (STREQ(snaplist->snaps[i].parent, names[j])) { + found = true; + if (!snaplist->snaps[j].parent) + VIR_FREE(snaplist->snaps[i].parent); + break; + } + } + if (!found) { + changed = true; + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + } + } + VIR_FREE(names[start_index]); + virDomainSnapshotFree(snaplist->snaps[start_index].snap); + snaplist->snaps[start_index].snap = NULL; + VIR_FREE(snaplist->snaps[start_index].parent); + deleted++; + } + if (roots) { + for (i = 0; i < count; i++) { + if (snaplist->snaps[i].parent) { + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + } + } + +success: + qsort(snaplist->snaps, count, sizeof(*snaplist->snaps), vshSnapSorter); + + snaplist->nsnaps -= deleted; + + ret = snaplist; + snaplist = NULL; + +cleanup: + vshSnapshotListFree(snaplist); + if (names) + for (i = 0; i < count; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + return ret; +} + /* * "snapshot-list" command */ -- 1.7.10.2
On 06/09/12 06:34, Eric Blake wrote:
This patch copies just the fallback code out of cmdSnapshotList, and keeps the snapshot objects around rather than just their name for easier manipulation. It looks forward to a future patch when we add a way to list all snapshot objects at once, and the next patch will simplify cmdSnapshotList to take advantage of this factorization.
* tools/virsh.c (vshSnapshotList, vshSnapshotListFree): New functions. --- tools/virsh.c | 272 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 272 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index de7b282..62546b2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c
+ +/* Compute a list of snapshots from DOM. If FROM is provided, the + * list is limited to descendants of the given snapshot. If FLAGS is + * given, the list is filtered. If TREE is specified, then the + * parents array of SNAPLIST will also be set in a manner usable by + * tree listings (note that parents may also be set when getting + * descendants using older servers). */ +static vshSnapshotListPtr ATTRIBUTE_UNUSED +vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, + virDomainSnapshotPtr from, + unsigned int flags, bool tree) +{ + int i; + char **names = NULL; + int count = 0; + bool descendants = false; + bool roots = false; + vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist)); + vshSnapshotListPtr ret = NULL; + const char *fromname = NULL; + int start_index = -1; + int deleted = 0; + + /* 0.9.13 will be adding a new listing API. */ + + /* This is the interface available in 0.9.12 and earlier, + * including fallbacks to 0.9.4 and earlier. */ + if (from) { + fromname = virDomainSnapshotGetName(from); + if (!fromname) { + vshError(ctl, "%s", _("Could not get snapshot name")); + goto cleanup; + } + descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0; + if (tree) + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + count = ctl->useSnapshotOld ? -1 : + virDomainSnapshotNumChildren(from, flags);
I'd prefer this to use an if statement instead from the ternary operator, this looks really messy. I tried reworking this block, please look below.
+ if (count < 0) { + if (ctl->useSnapshotOld || + last_error->code == VIR_ERR_NO_SUPPORT) {
Those two lines also aren't intuitive on the first read. last_error will be filled as virDomainSnapshotNumChildren has to fail or never be called.
+ /* We can emulate --from. */ + /* XXX can we also emulate --leaves? */ + virFreeError(last_error); + last_error = NULL; + ctl->useSnapshotOld = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + count = virDomainSnapshotNum(dom, flags); + } + } else if (tree) { + count++; + } + } else { + count = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + /* XXX can we also emulate --leaves? */ + if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + roots = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + count = virDomainSnapshotNum(dom, flags); + } + } + + if (count < 0) { + if (!last_error) + vshError(ctl, _("missing support"));
I know it's copied code, but this error message doesn't look helpful. I think it's worth improving: "Failed to collect snapshot list" perhaps?
+ goto cleanup; + }
I had a very hard time parsing the code flow in this block, I'd rewrite this block as follows: /* This is the interface available in 0.9.12 and earlier, * including fallbacks to 0.9.4 and earlier. */ if (from) { if (!(fromname = virDomainSnapshotGetName(from))) { vshError(ctl, "%s", _("Could not get snapshot name")); goto cleanup; } descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0; if (tree) flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; /* try new API at first */ if (!ctl->useSnapshotOld && (count = virDomainSnapshotNumChildren(from, flags)) < 0 && last_error && last_error->code == VIR_ERR_NO_SUPPORT) { /* We can emulate --from. */ /* XXX can we also emulate --leaves? */ virFreeError(last_error); lastError = NULL; ctl->useSnapshotOld = true; flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } if (tree && count >= 0) count++; } /* fallback to old API */ if (count < 0) { count = virDomainSnapshotNum(dom, flags); /* Fall back to simulation if --roots was unsupported. */ /* XXX can we also emulate --leaves? */ if (!from && count < 0 && last_error->code == VIR_ERR_INVALID_ARG && (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { virFreeError(last_error); last_error = NULL; roots = true; flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; count = virDomainSnapshotNum(dom, flags); } } if (count < 0) { if (!last_error) vshError(ctl, _("Failed to collect snapshot list")); goto cleanup; } if (!count) goto success;
+ + if (!count) + goto success; + + names = vshCalloc(ctl, sizeof(*names), count); + + if (from && !ctl->useSnapshotOld) { + if (tree) { + if (count) + count = virDomainSnapshotListChildrenNames(from, names + 1, + count - 1, flags); + if (count >= 0) { + count++; + names[0] = vshStrdup(ctl, fromname); + } + } else { + count = virDomainSnapshotListChildrenNames(from, names, + count, flags); + } + } else { + count = virDomainSnapshotListNames(dom, names, count, flags); + } + if (count < 0) + goto cleanup; + + snaplist->snaps = vshCalloc(ctl, sizeof(*snaplist->snaps), count); + snaplist->nsnaps = count; + for (i = 0; i < count; i++) { + snaplist->snaps[i].snap = virDomainSnapshotLookupByName(dom, + names[i], 0); + if (!snaplist->snaps[i].snap) + goto cleanup; + } + + if (tree || (from && ctl->useSnapshotOld)) { + for (i = (from && !ctl->useSnapshotOld); i < count; i++) { + if (from && ctl->useSnapshotOld && STREQ(names[i], fromname)) { + start_index = i; + continue; + } + if (vshGetSnapshotParent(ctl, snaplist->snaps[i].snap, + &snaplist->snaps[i].parent) < 0) + goto cleanup; + } + } + if (tree) { + if (from && ctl->useSnapshotOld) { + /* Leave things so that the only entry without a parent is + * 'from'. */ + for (i = 0; i < count; i++) { + if (STREQ(virDomainSnapshotGetName(snaplist->snaps[i].snap), + fromname)) { + VIR_FREE(snaplist->snaps[i].parent); + } else if (!snaplist->snaps[i].parent) { + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + deleted++; + } + } + } + goto success; + } + + if (ctl->useSnapshotOld && descendants) { + bool changed = false; + + /* Make multiple passes over the list - first pass NULLs out + * all roots except start, remaining passes NULL out any entry + * whose parent is not still in list. Also, we NULL out + * parent when name is known to be in list. Sorry, this is + * O(n^3) - hope your hierarchy isn't huge. */ + if (start_index < 0) { + vshError(ctl, _("snapshot %s disappeared from list"), fromname); + goto cleanup; + } + for (i = 0; i < count; i++) { + if (i == start_index) + continue; + if (!snaplist->snaps[i].parent) { + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } else if (STREQ(snaplist->snaps[i].parent, fromname)) { + VIR_FREE(snaplist->snaps[i].parent); + changed = true; + } + } + if (!changed) + goto success; + while (changed) { + changed = false; + for (i = 0; i < count; i++) { + bool found = false; + int j; + + if (!names[i] || !snaplist->snaps[i].parent) + continue; + for (j = 0; j < count; j++) { + if (!names[j] || i == j) + continue; + if (STREQ(snaplist->snaps[i].parent, names[j])) { + found = true; + if (!snaplist->snaps[j].parent) + VIR_FREE(snaplist->snaps[i].parent); + break; + } + } + if (!found) { + changed = true; + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + } + } + VIR_FREE(names[start_index]); + virDomainSnapshotFree(snaplist->snaps[start_index].snap); + snaplist->snaps[start_index].snap = NULL; + VIR_FREE(snaplist->snaps[start_index].parent); + deleted++; + } + if (roots) { + for (i = 0; i < count; i++) { + if (snaplist->snaps[i].parent) { + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + } + } + +success: + qsort(snaplist->snaps, count, sizeof(*snaplist->snaps), vshSnapSorter); + + snaplist->nsnaps -= deleted; + + ret = snaplist; + snaplist = NULL; + +cleanup: + vshSnapshotListFree(snaplist); + if (names) + for (i = 0; i < count; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + return ret; +} +
This one was really though to review. As this is just code movement and your tweaks to use the new structs look correct I'm fine if you push it without change. Peter
On 06/12/12 14:54, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
This patch copies just the fallback code out of cmdSnapshotList, and keeps the snapshot objects around rather than just their name for easier manipulation. It looks forward to a future patch when we add a way to list all snapshot objects at once, and the next patch will simplify cmdSnapshotList to take advantage of this factorization.
I know it's copied code, but this error message doesn't look helpful. I think it's worth improving: "Failed to collect snapshot list" perhaps?
+ goto cleanup; + }
I had a very hard time parsing the code flow in this block, I'd rewrite this block as follows:
I forgot to attach the patch for the rewrite. Peter
On 06/12/2012 06:59 AM, Peter Krempa wrote:
On 06/12/12 14:54, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
This patch copies just the fallback code out of cmdSnapshotList, and keeps the snapshot objects around rather than just their name for easier manipulation. It looks forward to a future patch when we add a way to list all snapshot objects at once, and the next patch will simplify cmdSnapshotList to take advantage of this factorization.
I know it's copied code, but this error message doesn't look helpful. I think it's worth improving: "Failed to collect snapshot list" perhaps?
+ goto cleanup; + }
I had a very hard time parsing the code flow in this block, I'd rewrite this block as follows:
I forgot to attach the patch for the rewrite.
Thanks; that helps.
@@ -16641,36 +16641,40 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, /* This is the interface available in 0.9.12 and earlier, * including fallbacks to 0.9.4 and earlier. */
I'm off on my versioning - this was 0.9.6 and earlier.
if (from) {
There's multiple things going on here: First, the valid combinations: list (all snapshots, no parent info needed) list --roots (only root snapshots) list --from (only direct children of --from) list --from --descendants (recursive children of --from) list --tree (all snapshots, parent info needed) list --tree --from (recursive children of --from, but also list from) Then the API that satisfy those combinations: 'new' API (aka 0.9.7-0.9.12) with --from (regardless of --descendants) -> use virDomainSnapshotNumChildren in its entirety new API without --from (regardless of -roots) -> use virDomainSnapshotNum in its entirety 'old' API (aka 0.8.0-0.9.6) without --from or --roots -> use virDomainSnapshotNum in its entirety old API without --from but with --roots -> use virDomainSnapshotNum, but then manually filter to just roots old with --from (regardless of --descendants) -> use virDomainSnapshotNum to get a global list, then manually filter to just the snapshot's children Additionally, if both --from and --tree are present, then we want --from to be included in the list. I really need to list this as a comment. I'll post a v2, adding comments and including your improvements.
+ /* fallback to old API */ + if (count < 0) { count = virDomainSnapshotNum(dom, flags);
More like: fall back to old API when doing children listing, or do initial probe when doing global listing. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/12/2012 05:27 PM, Eric Blake wrote:
I'm off on my versioning - this was 0.9.6 and earlier.
if (from) {
There's multiple things going on here:
First, the valid combinations:
Then the API that satisfy those combinations:
I really need to list this as a comment. I'll post a v2, adding comments and including your improvements.
Here's what I'll squash for my v2: diff --git i/tools/virsh.c w/tools/virsh.c index 768d189..537496f 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -16848,7 +16848,7 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, { int i; char **names = NULL; - int count = 0; + int count = -1; bool descendants = false; bool roots = false; vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist)); @@ -16859,8 +16859,24 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, /* 0.9.13 will be adding a new listing API. */ - /* This is the interface available in 0.9.12 and earlier, - * including fallbacks to 0.9.4 and earlier. */ + /* This uses the interfaces available in 0.8.0-0.9.6 + * (virDomainSnapshotListNames, global list only) and in + * 0.9.7-0.9.12 (addition of virDomainSnapshotListChildrenNames + * for child listing, and new flags), as follows, with [*] by the + * combinations that need parent info (either for filtering + * purposes or for the resulting tree listing): + * old new + * list global as-is global as-is + * list --roots *global + filter global + flags + * list --from *global + filter child + flags + * list --from --descendants *global + filter child + flags + * list --tree *global as-is *global as-is + * list --tree --from *global + filter *child + flags + * + * Additionally, when --tree and --from are both used, from is + * added to the final list as the only element without a parent. + * Otherwise, --from does not appear in the final list. + */ if (from) { fromname = virDomainSnapshotGetName(from); if (!fromname) { @@ -16870,28 +16886,30 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0; if (tree) flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - count = ctl->useSnapshotOld ? -1 : - virDomainSnapshotNumChildren(from, flags); - if (count < 0) { - if (ctl->useSnapshotOld || - last_error->code == VIR_ERR_NO_SUPPORT) { - /* We can emulate --from. */ - /* XXX can we also emulate --leaves? */ - virFreeError(last_error); - last_error = NULL; - ctl->useSnapshotOld = true; - flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - count = virDomainSnapshotNum(dom, flags); - } - } else if (tree) { - count++; + + /* First, determine if we can use the new child listing API. */ + if (!ctl->useSnapshotOld && + (count = virDomainSnapshotNumChildren(from, flags) < 0) && + last_error->code == VIR_ERR_NO_SUPPORT) { + /* We can emulate --from. */ + /* XXX can we also emulate --leaves? */ + virFreeError(last_error); + last_error = NULL; + ctl->useSnapshotOld = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } - } else { + if (tree && count >= 0) + count++; + } + + /* Global listing (including fallback when --from failed with + * child listing). */ + if (count < 0) { count = virDomainSnapshotNum(dom, flags); /* Fall back to simulation if --roots was unsupported. */ /* XXX can we also emulate --leaves? */ - if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG && + if (!from && count < 0 && last_error->code == VIR_ERR_INVALID_ARG && (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { virFreeError(last_error); last_error = NULL; @@ -16903,7 +16921,7 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, if (count < 0) { if (!last_error) - vshError(ctl, _("missing support")); + vshError(ctl, _("failed to collect snapshot list")); goto cleanup; } @@ -16912,6 +16930,7 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, names = vshCalloc(ctl, sizeof(*names), count); + /* Now that we have a count, collect the list. */ if (from && !ctl->useSnapshotOld) { if (tree) { if (count) @@ -16940,6 +16959,10 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, goto cleanup; } + /* Collect parents when needed. With the new API, --tree and + * --from together put from as the first element without a parent; + * with the old API we still need to do a post-process filtering + * based on all parent information. */ if (tree || (from && ctl->useSnapshotOld)) { for (i = (from && !ctl->useSnapshotOld); i < count; i++) { if (from && ctl->useSnapshotOld && STREQ(names[i], fromname)) { -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/12/2012 06:42 PM, Eric Blake wrote:
I really need to list this as a comment. I'll post a v2, adding comments and including your improvements.
Here's what I'll squash for my v2:
+ /* First, determine if we can use the new child listing API. */ + if (!ctl->useSnapshotOld && + (count = virDomainSnapshotNumChildren(from, flags) < 0) &&
Ouch - nasty typo on my part in transcribing Peter's suggestions (hint - ')' in the wrong place). All the more reason for me to post a corrected v2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Operating on a list of snapshot objects looks so much simpler. In particular, since the helper function already trimmed out irrelevant entries, we no longer have quite so many special cases on finding the first snapshot to operate on. * tools/virsh.c (cmdSnapshotList): Use previous patches. --- tools/virsh.c | 207 +++++++++------------------------------------------------ 1 file changed, 31 insertions(+), 176 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 62546b2..dc098f5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16621,7 +16621,7 @@ vshSnapSorter(const void *a, const void *b) * parents array of SNAPLIST will also be set in a manner usable by * tree listings (note that parents may also be set when getting * descendants using older servers). */ -static vshSnapshotListPtr ATTRIBUTE_UNUSED +static vshSnapshotListPtr vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, virDomainSnapshotPtr from, unsigned int flags, bool tree) @@ -16840,6 +16840,15 @@ cleanup: return ret; } +static const char * +vshSnapshotListLookup(int id, bool parent, void *opaque) +{ + vshSnapshotListPtr snaplist = opaque; + if (parent) + return snaplist->snaps[id].parent; + return virDomainSnapshotGetName(snaplist->snaps[id].snap); +} + /* * "snapshot-list" command */ @@ -16870,11 +16879,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; unsigned int flags = 0; - int parent_filter = 0; /* -1 for roots filtering, 0 for no parent - information needed, 1 for parent column */ - char **names = NULL; - char **parents = NULL; - int count = 0; + bool show_parent = false; int i; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; @@ -16890,8 +16895,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) bool leaves = vshCommandOptBool(cmd, "leaves"); const char *from = NULL; virDomainSnapshotPtr start = NULL; - int start_index = -1; bool descendants = false; + vshSnapshotListPtr snaplist = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -16916,7 +16921,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) _("--parent and --tree are mutually exclusive")); goto cleanup; } - parent_filter = 1; + show_parent = true; } else if (vshCommandOptBool(cmd, "roots")) { if (tree) { vshError(ctl, "%s", @@ -16945,48 +16950,16 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (from) { descendants = vshCommandOptBool(cmd, "descendants"); - if (descendants || tree) { + if (descendants) flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - } - count = ctl->useSnapshotOld ? -1 : - virDomainSnapshotNumChildren(start, flags); - if (count < 0) { - if (ctl->useSnapshotOld || - last_error->code == VIR_ERR_NO_SUPPORT) { - /* We can emulate --from. */ - /* XXX can we also emulate --leaves? */ - virFreeError(last_error); - last_error = NULL; - ctl->useSnapshotOld = true; - flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - count = virDomainSnapshotNum(dom, flags); - } - } else if (tree) { - count++; - } - } else { - count = virDomainSnapshotNum(dom, flags); - - /* Fall back to simulation if --roots was unsupported. */ - /* XXX can we also emulate --leaves? */ - if (count < 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; - count = virDomainSnapshotNum(dom, flags); - } } - if (count < 0) { - if (!last_error) - vshError(ctl, _("missing support")); + if ((snaplist = vshSnapshotListCollect(ctl, dom, start, flags, + tree)) == NULL) goto cleanup; - } if (!tree) { - if (parent_filter > 0) + if (show_parent) vshPrintExtra(ctl, " %-20s %-25s %-15s %s", _("Name"), _("Creation Time"), _("State"), _("Parent")); @@ -16997,142 +16970,35 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) "------------------------------------------------------------\n"); } - if (!count) { + if (!snaplist->nsnaps) { ret = true; goto cleanup; } - if (VIR_ALLOC_N(names, count) < 0) - goto cleanup; - - 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) { - if (count) - count = virDomainSnapshotListChildrenNames(start, names + 1, - count - 1, flags); - if (count >= 0) { - count++; - names[0] = vshStrdup(ctl, from); - } - } else { - count = virDomainSnapshotListChildrenNames(start, names, - count, flags); - } - } else { - count = virDomainSnapshotListNames(dom, names, count, flags); - } - if (count < 0) - goto cleanup; - - qsort(&names[0], count, sizeof(char*), namesorter); - - if (tree || (from && ctl->useSnapshotOld)) { - parents = vshCalloc(ctl, sizeof(char *), count); - for (i = (from && !ctl->useSnapshotOld); i < count; i++) { - if (from && ctl->useSnapshotOld && STREQ(names[i], from)) { - start_index = i; - continue; - } - - /* free up memory from previous iterations of the loop */ - if (snapshot) - virDomainSnapshotFree(snapshot); - snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); - if (!snapshot || - vshGetSnapshotParent(ctl, snapshot, &parents[i]) < 0) { - while (--i >= 0) - VIR_FREE(parents[i]); - VIR_FREE(parents); - goto cleanup; - } - } - } if (tree) { - struct vshTreeArray arrays = { names, parents }; - - for (i = 0 ; i < count ; i++) { - if ((from && ctl->useSnapshotOld) ? STREQ(names[i], from) : - !parents[i] && - vshTreePrint(ctl, vshTreeArrayLookup, &arrays, count, i) < 0) + for (i = 0 ; i < snaplist->nsnaps ; i++) { + if (!snaplist->snaps[i].parent && + vshTreePrint(ctl, vshSnapshotListLookup, snaplist, + snaplist->nsnaps, i) < 0) goto cleanup; } - ret = true; goto cleanup; } - if (ctl->useSnapshotOld && descendants) { - bool changed = false; - - /* Make multiple passes over the list - first pass NULLs out - * all roots except start, remaining passes NULL out any entry - * whose parent is not still in list. Also, we NULL out - * parent when name is known to be in list. Sorry, this is - * O(n^3) - hope your hierarchy isn't huge. */ - if (start_index < 0) { - vshError(ctl, _("snapshot %s disappeared from list"), from); - goto cleanup; - } - for (i = 0; i < count; i++) { - if (i == start_index) - continue; - if (!parents[i]) { - VIR_FREE(names[i]); - } else if (STREQ(parents[i], from)) { - VIR_FREE(parents[i]); - changed = true; - } - } - if (!changed) { - ret = true; - goto cleanup; - } - while (changed) { - changed = false; - for (i = 0; i < count; i++) { - bool found = false; - int j; - - if (!names[i] || !parents[i]) - continue; - for (j = 0; j < count; j++) { - if (!names[j] || i == j) - continue; - if (STREQ(parents[i], names[j])) { - found = true; - if (!parents[j]) - VIR_FREE(parents[i]); - break; - } - } - if (!found) { - changed = true; - VIR_FREE(names[i]); - } - } - } - VIR_FREE(names[start_index]); - } - - for (i = 0; i < count; i++) { - if (from && ctl->useSnapshotOld && - (descendants ? !names[i] : STRNEQ_NULLABLE(parents[i], from))) - continue; + for (i = 0; i < snaplist->nsnaps; i++) { + const char *name; /* free up memory from previous iterations of the loop */ VIR_FREE(parent); VIR_FREE(state); - if (snapshot) - virDomainSnapshotFree(snapshot); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); - if (snapshot == NULL) - continue; + snapshot = snaplist->snaps[i].snap; + name = virDomainSnapshotGetName(snapshot); + assert(name); doc = virDomainSnapshotGetXMLDesc(snapshot, 0); if (!doc) @@ -17142,12 +17008,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (!xml) continue; - if (parent_filter) { + if (show_parent) parent = virXPathString("string(/domainsnapshot/parent/name)", ctxt); - if (!parent && parent_filter < 0) - continue; - } state = virXPathString("string(/domainsnapshot/state)", ctxt); if (state == NULL) @@ -17166,31 +17029,23 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (parent) vshPrint(ctl, " %-20s %-25s %-15s %s\n", - names[i], timestr, state, parent); + name, timestr, state, parent); else - vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); + vshPrint(ctl, " %-20s %-25s %s\n", name, timestr, state); } ret = true; cleanup: /* this frees up memory from the last iteration of the loop */ + vshSnapshotListFree(snaplist); VIR_FREE(parent); VIR_FREE(state); - if (snapshot) - virDomainSnapshotFree(snapshot); if (start) virDomainSnapshotFree(start); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - for (i = 0; i < count; i++) { - VIR_FREE(names[i]); - if (parents) - VIR_FREE(parents[i]); - } - VIR_FREE(names); - VIR_FREE(parents); if (dom) virDomainFree(dom); -- 1.7.10.2
On 06/09/12 06:34, Eric Blake wrote:
Operating on a list of snapshot objects looks so much simpler. In particular, since the helper function already trimmed out irrelevant entries, we no longer have quite so many special cases on finding the first snapshot to operate on.
* tools/virsh.c (cmdSnapshotList): Use previous patches. --- tools/virsh.c | 207 +++++++++------------------------------------------------ 1 file changed, 31 insertions(+), 176 deletions(-)
ACK. Peter
On 06/12/2012 07:18 AM, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
Operating on a list of snapshot objects looks so much simpler. In particular, since the helper function already trimmed out irrelevant entries, we no longer have quite so many special cases on finding the first snapshot to operate on.
* tools/virsh.c (cmdSnapshotList): Use previous patches. --- tools/virsh.c | 207 +++++++++------------------------------------------------ 1 file changed, 31 insertions(+), 176 deletions(-)
ACK.
As mentioned in 2/6, I'll squash this in for my v2, now that the callback struct is only used by one client instead of generic to vshTreePrint: diff --git i/tools/virsh.c w/tools/virsh.c index 84db8d3..c7877b3 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -13473,15 +13473,15 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, return ret; } -struct vshTreeArray { +struct vshNodeList { char **names; char **parents; }; static const char * -vshTreeArrayLookup(int devid, bool parent, void *opaque) +vshNodeListLookup(int devid, bool parent, void *opaque) { - struct vshTreeArray *arrays = opaque; + struct vshNodeList *arrays = opaque; if (parent) return arrays->parents[devid]; return arrays->names[devid]; @@ -13536,7 +13536,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) qsort(&devices[0], num_devices, sizeof(char*), namesorter); if (tree) { char **parents = vshMalloc(ctl, sizeof(char *) * num_devices); - struct vshTreeArray arrays = { devices, parents }; + struct vshNodeList arrays = { devices, parents }; for (i = 0; i < num_devices; i++) { virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]); @@ -13550,7 +13550,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } for (i = 0 ; i < num_devices ; i++) { if (parents[i] == NULL && - vshTreePrint(ctl, vshTreeArrayLookup, &arrays, num_devices, + vshTreePrint(ctl, vshNodeListLookup, &arrays, num_devices, i) < 0) ret = false; } -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa