[libvirt] [PATCHv2 03/13] snapshot: use metaroot node to simplify management

[Peter Krempa]

On 06/15/12 06:18, Eric Blake wrote:
> This idea was first suggested by Daniel Veillard here:
> https://www.redhat.com/archives/libvir-list/2011-October/msg00353.html
>
> Now that I am about to add more complexity to snapshot listing, it
> makes sense to avoid code duplication and special casing for domain
> listing (all snapshots) vs. snapshot listing (descendants); adding
> a metaroot reduces the number of code lines by having the domain
> listing turn into a descendant listing of the metaroot.
>
> Note that this has one minor pessimization - if we are going to list
> ALL snapshots without filtering, then virHashForeach is more efficient
> than recursing through the child relationships; restoring that minor
> optimization will occur in the next patch.
>
> * src/conf/domain_conf.h (_virDomainSnapshotObj)
> (_virDomainSnapshotObjList): Repurpose some fields.
> (virDomainSnapshotDropParent): Drop unused parameter.
> * src/conf/domain_conf.c (virDomainSnapshotObjListGetNames)
> (virDomainSnapshotObjListCount): Simplify.
> (virDomainSnapshotFindByName, virDomainSnapshotSetRelations)
> (virDomainSnapshotDropParent): Match new field semantics.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML)
> (qemuDomainSnapshotReparentChildren, qemuDomainSnapshotDelete):
> Adjust clients.
> ---
>
> v2: fix bugs in virDomainSnapshotSetRelations, rebase earlier in series
>
>   src/conf/domain_conf.c |  116 ++++++++++++------------------------------------
>   src/conf/domain_conf.h |   12 ++---
>   src/qemu/qemu_driver.c |   36 +++++----------
>   3 files changed, 46 insertions(+), 118 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8d80f3b..c7437af 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14289,32 +14289,9 @@ int virDomainSnapshotObjListGetNames(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_ROOTS;
> -
> -    if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
> -        virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames,
> -                       &data);
> -    } else {
> -        virDomainSnapshotObjPtr root = snapshots->first_root;
> -        while (root) {
> -            virDomainSnapshotObjListCopyNames(root, root->def->name, &data);
> -            root = root->sibling;
> -        }
> -    }
> -    if (data.oom) {
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    return data.numnames;
> -
> -cleanup:
> -    for (i = 0; i < data.numnames; i++)
> -        VIR_FREE(data.names[i]);
> -    return -1;
> +    flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;

I'm not quite sure what's the meaning of this statement. It efectively 
negates the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS flag, but the original code 
filtered it out.

> +    return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names,
> +                                                maxnames, flags);

This function calls virDomainSnapshotObjListCopyNames on each of 
children of the object but the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS is never 
checked. In fact virDomainSnapshotObjListCopyNames states that:

  "/* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, ..."

I assume that it has to be filtered out:
flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;


>   }
>
>   int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot,
> @@ -14369,23 +14346,8 @@ static void virDomainSnapshotObjListCount(void *payload,
>   int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
>                                   unsigned int flags)
>   {
> -    struct virDomainSnapshotNumData data = { 0, 0 };
> -
> -    data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
> -
> -    if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
> -        virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data);
> -    } else if (data.flags) {
> -        virDomainSnapshotObjPtr root = snapshots->first_root;
> -        while (root) {
> -            virDomainSnapshotObjListCount(root, root->def->name, &data);
> -            root = root->sibling;
> -        }
> -    } else {
> -        data.count = snapshots->nroots;
> -    }
> -
> -    return data.count;
> +    flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
> +    return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags);

Same comment as above.

>   }
>
>   int


ACK if the flag masking issue gets cleared. The metaroot approach is 
nice and consistent.

Peter