[libvirt] [PATCH 07/16] snapshot: Add accessors for updating snapshot list relations

[John Ferlan]

On 3/20/19 1:40 AM, Eric Blake wrote:
> Rather than allowing a leaky abstraction where multiple drivers have
> to open-code operations that update the relations in a
> virDomainSnapshotObjList, it is better to add accessor functions so
> that updates to relations are maintained closer to the internals.  The
> goal here is to avoid access to, nchildren, first_child, or sibling
> outside of the lowest level functions, making it easier to refactor
> later on.  While many of the conversions are fairly straightforward,
> the MoveChildren refactoring can be a bit interesting to follow.

Understatement ;-)  Some black magic occurs

The qemuDomainSnapshotReparentChildren "snap->parent = rep->parent" gets
replaced by the new for loop... Tough to see without watching really
closely.

> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/virdomainsnapshotobj.h     |  5 ++++
>  src/conf/virdomainsnapshotobjlist.h |  2 ++
>  src/conf/virdomainsnapshotobj.c     | 42 +++++++++++++++++++++++++++++
>  src/conf/virdomainsnapshotobjlist.c | 42 ++++++++++++++++++-----------
>  src/libvirt_private.syms            |  3 +++
>  src/qemu/qemu_driver.c              | 19 +++----------
>  src/test/test_driver.c              | 18 +++----------
>  7 files changed, 85 insertions(+), 46 deletions(-)
> 

[...]

> diff --git a/src/conf/virdomainsnapshotobj.c b/src/conf/virdomainsnapshotobj.c
> index 7f92ac21d9..d6b216c7b2 100644
> --- a/src/conf/virdomainsnapshotobj.c
> +++ b/src/conf/virdomainsnapshotobj.c

[...]

> +/* Take all children of @from and convert them into children of @to. */
> +void
> +virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from,
> +                              virDomainSnapshotObjPtr to)
> +{
> +    virDomainSnapshotObjPtr child;
> +    virDomainSnapshotObjPtr last;
> +
> +    for (child = from->first_child; child; child = child->sibling) {
> +        child->parent = to;
> +        if (!child->sibling)
> +            last = child;
> +    }
> +    to->nchildren += from->nchildren;
> +    last->sibling = to->first_child;

Silly Coverity compiler gets quite confused thinking that @last couldn't
be set while not considering the above loop couldn't end without it
unless of course from->first_child == NULL I suppose, which would be a
different issue. Still if before the for loop we check "if
(!from->first_child) return;", then coverity is happy.

> +    to->first_child = from->first_child;
> +    from->nchildren = 0;
> +    from->first_child = NULL;

Or virDomainSnapshotDropChildren... Still makes sense, albeit new. Seems
to make VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY processing unnecessary.
It's not a problem but perhaps worthy of a mention.

> +}
> diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
> index 1eecb89a5d..9538521ab3 100644
> --- a/src/conf/virdomainsnapshotobjlist.c
> +++ b/src/conf/virdomainsnapshotobjlist.c
> @@ -72,7 +72,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
>                         _("incorrect flags for bulk parse"));
>          return -1;
>      }
> -    if (snapshots->metaroot.nchildren || snapshots->current) {
> +    if (virDomainSnapshotObjListSize(snapshots)) {

The only way the call could return < 0 from virHashSize is if @snapshots
== NULL...  Just noting it - no problem.

>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("bulk define of snapshots only possible with "
>                           "no existing snapshot"));

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

BTW: There could have been 3 patches out of this, but I'm fine with 1.