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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list