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.

Signed-off-by: Eric Blake <eblake@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.h b/src/conf/virdomainsnapshotobj.h
index 957f1b2ea8..0981ea4c25 100644
--- a/src/conf/virdomainsnapshotobj.h
+++ b/src/conf/virdomainsnapshotobj.h
@@ -46,5 +46,10 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot,
                                        virHashIterator iter,
                                        void *data);
 void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotDropChildren(virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from,
+                                   virDomainSnapshotObjPtr to);
+void virDomainSnapshotSetParent(virDomainSnapshotObjPtr snapshot,
+                                virDomainSnapshotObjPtr parent);

 #endif /* LIBVIRT_VIRDOMAINSNAPSHOTOBJ_H */
diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index e210849441..c13a0b4026 100644
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -55,6 +55,7 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
                                 unsigned int flags);
 virDomainSnapshotObjPtr virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots,
                                                     const char *name);
+int virDomainSnapshotObjListSize(virDomainSnapshotObjListPtr snapshots);
 virDomainSnapshotObjPtr virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots);
 const char *virDomainSnapshotGetCurrentName(virDomainSnapshotObjListPtr snapshots);
 bool virDomainSnapshotIsCurrentName(virDomainSnapshotObjListPtr snapshots,
@@ -63,6 +64,7 @@ void virDomainSnapshotSetCurrent(virDomainSnapshotObjListPtr snapshots,
                                  virDomainSnapshotObjPtr snapshot);
 bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
                                     virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots);
 int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
                              virHashIterator iter,
                              void *data);
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
@@ -121,3 +121,45 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot)
     snapshot->parent = NULL;
     snapshot->sibling = NULL;
 }
+
+
+/* Update @snapshot to no longer have children. */
+void
+virDomainSnapshotDropChildren(virDomainSnapshotObjPtr snapshot)
+{
+    snapshot->nchildren = 0;
+    snapshot->first_child = NULL;
+}
+
+
+/* Add @snapshot to @parent's list of children. */
+void
+virDomainSnapshotSetParent(virDomainSnapshotObjPtr snapshot,
+                           virDomainSnapshotObjPtr parent)
+{
+    snapshot->parent = parent;
+    parent->nchildren++;
+    snapshot->sibling = parent->first_child;
+    parent->first_child = snapshot;
+}
+
+
+/* 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;
+    to->first_child = from->first_child;
+    from->nchildren = 0;
+    from->first_child = NULL;
+}
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)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("bulk define of snapshots only possible with "
                          "no existing snapshot"));
@@ -143,9 +143,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
     if (ret < 0) {
         /* There were no snapshots before this call; so on error, just
          * blindly delete anything created before the failure. */
-        virHashRemoveAll(snapshots->objs);
-        snapshots->metaroot.nchildren = 0;
-        snapshots->metaroot.first_child = NULL;
+        virDomainSnapshotObjListRemoveAll(snapshots);
     }
     xmlXPathFreeContext(ctxt);
     xmlFreeDoc(xml);
@@ -437,6 +435,14 @@ virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots,
 }


+/* Return the number of objects currently in the list */
+int
+virDomainSnapshotObjListSize(virDomainSnapshotObjListPtr snapshots)
+{
+    return virHashSize(snapshots->objs);
+}
+
+
 /* Return the current snapshot, or NULL */
 virDomainSnapshotObjPtr
 virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots)
@@ -484,6 +490,15 @@ bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
     return ret;
 }

+/* Remove all snapshots tracked in the list */
+void
+virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots)
+{
+    virHashRemoveAll(snapshots->objs);
+    virDomainSnapshotDropChildren(&snapshots->metaroot);
+}
+
+
 int
 virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
                          virHashIterator iter,
@@ -511,28 +526,26 @@ virDomainSnapshotSetRelations(void *payload,
     virDomainSnapshotObjPtr obj = payload;
     struct snapshot_set_relation *curr = data;
     virDomainSnapshotObjPtr tmp;
+    virDomainSnapshotObjPtr parent;

-    obj->parent = virDomainSnapshotFindByName(curr->snapshots,
-                                              obj->def->parent);
-    if (!obj->parent) {
+    parent = virDomainSnapshotFindByName(curr->snapshots, obj->def->parent);
+    if (!parent) {
         curr->err = -1;
-        obj->parent = &curr->snapshots->metaroot;
+        parent = &curr->snapshots->metaroot;
         VIR_WARN("snapshot %s lacks parent", obj->def->name);
     } else {
-        tmp = obj->parent;
+        tmp = parent;
         while (tmp && tmp->def) {
             if (tmp == obj) {
                 curr->err = -1;
-                obj->parent = &curr->snapshots->metaroot;
+                parent = &curr->snapshots->metaroot;
                 VIR_WARN("snapshot %s in circular chain", obj->def->name);
                 break;
             }
             tmp = tmp->parent;
         }
     }
-    obj->parent->nchildren++;
-    obj->sibling = obj->parent->first_child;
-    obj->parent->first_child = obj;
+    virDomainSnapshotSetParent(obj, parent);
     return 0;
 }

@@ -545,8 +558,7 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots)
 {
     struct snapshot_set_relation act = { snapshots, 0 };

-    snapshots->metaroot.nchildren = 0;
-    snapshots->metaroot.first_child = NULL;
+    virDomainSnapshotDropChildren(&snapshots->metaroot);
     virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act);
     if (act.err)
         snapshots->current = NULL;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 72c5cef528..ffc1724850 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -980,9 +980,12 @@ virDomainObjListRename;


 # conf/virdomainsnapshotobj.h
+virDomainSnapshotDropChildren;
 virDomainSnapshotDropParent;
 virDomainSnapshotForEachChild;
 virDomainSnapshotForEachDescendant;
+virDomainSnapshotMoveChildren;
+virDomainSnapshotSetParent;


 # conf/virdomainsnapshotobjlist.h
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6c71382b93..eb3d112b69 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15926,10 +15926,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
         } else {
             other = virDomainSnapshotFindByName(vm->snapshots,
                                                 snap->def->parent);
-            snap->parent = other;
-            other->nchildren++;
-            snap->sibling = other->first_child;
-            other->first_child = snap;
+            virDomainSnapshotSetParent(snap, other);
         }
     } else if (snap) {
         virDomainSnapshotObjListRemove(vm->snapshots, snap);
@@ -16763,7 +16760,6 @@ struct _virQEMUSnapReparent {
     virCapsPtr caps;
     virDomainXMLOptionPtr xmlopt;
     int err;
-    virDomainSnapshotObjPtr last;
 };


@@ -16779,7 +16775,6 @@ qemuDomainSnapshotReparentChildren(void *payload,
         return 0;

     VIR_FREE(snap->def->parent);
-    snap->parent = rep->parent;

     if (rep->parent->def &&
         VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
@@ -16787,9 +16782,6 @@ qemuDomainSnapshotReparentChildren(void *payload,
         return 0;
     }

-    if (!snap->sibling)
-        rep->last = snap;
-
     rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap,
                                                rep->caps, rep->xmlopt,
                                                rep->cfg->snapshotDir);
@@ -16877,7 +16869,6 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
         rep.parent = snap->parent;
         rep.vm = vm;
         rep.err = 0;
-        rep.last = NULL;
         rep.caps = driver->caps;
         rep.xmlopt = driver->xmlopt;
         virDomainSnapshotForEachChild(snap,
@@ -16885,15 +16876,11 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
                                       &rep);
         if (rep.err < 0)
             goto endjob;
-        /* Can't modify siblings during ForEachChild, so do it now.  */
-        snap->parent->nchildren += snap->nchildren;
-        rep.last->sibling = snap->parent->first_child;
-        snap->parent->first_child = snap->first_child;
+        virDomainSnapshotMoveChildren(snap, snap->parent);
     }

     if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
-        snap->nchildren = 0;
-        snap->first_child = NULL;
+        virDomainSnapshotDropChildren(snap);
         ret = 0;
     } else {
         ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 9cbef70f1c..d3b76bfdbd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6416,10 +6416,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
                 virDomainSnapshotSetCurrent(vm->snapshots, snap);
             other = virDomainSnapshotFindByName(vm->snapshots,
                                                 snap->def->parent);
-            snap->parent = other;
-            other->nchildren++;
-            snap->sibling = other->first_child;
-            other->first_child = snap;
+            virDomainSnapshotSetParent(snap, other);
         }
         virDomainObjEndAPI(&vm);
     }
@@ -6454,7 +6451,6 @@ struct _testSnapReparentData {
     virDomainSnapshotObjPtr parent;
     virDomainObjPtr vm;
     int err;
-    virDomainSnapshotObjPtr last;
 };

 static int
@@ -6469,7 +6465,6 @@ testDomainSnapshotReparentChildren(void *payload,
         return 0;

     VIR_FREE(snap->def->parent);
-    snap->parent = rep->parent;

     if (rep->parent->def &&
         VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
@@ -6477,8 +6472,6 @@ testDomainSnapshotReparentChildren(void *payload,
         return 0;
     }

-    if (!snap->sibling)
-        rep->last = snap;
     return 0;
 }

@@ -6515,22 +6508,17 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
         rep.parent = snap->parent;
         rep.vm = vm;
         rep.err = 0;
-        rep.last = NULL;
         virDomainSnapshotForEachChild(snap,
                                       testDomainSnapshotReparentChildren,
                                       &rep);
         if (rep.err < 0)
             goto cleanup;

-        /* Can't modify siblings during ForEachChild, so do it now.  */
-        snap->parent->nchildren += snap->nchildren;
-        rep.last->sibling = snap->parent->first_child;
-        snap->parent->first_child = snap->first_child;
+        virDomainSnapshotMoveChildren(snap, snap->parent);
     }

     if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
-        snap->nchildren = 0;
-        snap->first_child = NULL;
+        virDomainSnapshotDropChildren(snap);
     } else {
         virDomainSnapshotDropParent(snap);
         if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com>

John

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 3/20/19 4:28 PM, John Ferlan wrote:
> 
> 
> 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.
> 

>> +/* 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.

Good find from Coverity. If there are no children to move, I do need the
early exit, so I'll squash that in.

> 
>> +    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.

I'll call out more details in the commit message.

>> -    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.

Yeah, we don't want to continue with either a -1 error (which shouldn't
happen in practice) or a > 0 result (more likely); but adding '!= 0'
does appear to make it clearer that I thought about both directions.

> 
>>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>                         _("bulk define of snapshots only possible with "
>>                           "no existing snapshot"));
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> BTW: There could have been 3 patches out of this, but I'm fine with 1.

I ended up splitting into two - everything else is simple, and then
MoveChildren in isolation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Eric Blake <eblake@redhat.com> [2019-03-21, 01:03PM -0500]:
> On 3/20/19 4:28 PM, John Ferlan wrote:
> > 
> > 
> > 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.
> > 
> 
> >> +/* 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.
> 
> Good find from Coverity. If there are no children to move, I do need the
> early exit, so I'll squash that in.

Did you forget this? Function is pushed in this (broken?) version and I
get a warning/error on GCC 8.0.1.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Bjoern Walk <bwalk@linux.ibm.com> [2019-03-28, 08:31AM +0100]:
> Eric Blake <eblake@redhat.com> [2019-03-21, 01:03PM -0500]:
> > On 3/20/19 4:28 PM, John Ferlan wrote:
> > > 
> > > 
> > > 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.
> > > 
> > 
> > >> +/* 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.
> > 
> > Good find from Coverity. If there are no children to move, I do need the
> > early exit, so I'll squash that in.
> 
> Did you forget this? Function is pushed in this (broken?) version and I
> get a warning/error on GCC 8.0.1.

Sorry, no morning coffee yet, the fix is in, but I still get a GCC
warning:

	  CC       conf/libvirt_conf_la-virdomainmomentobjlist.lo
	../../src/conf/virdomainmomentobjlist.c: In function 'virDomainMomentMoveChildren':
	../../src/conf/virdomainmomentobjlist.c:178:19: error: 'last' may be used uninitialized in this function [-Werror=maybe-uninitialized]
		 last->sibling = to->first_child;
		 ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
	cc1: all warnings being treated as errors
	make[5]: *** [Makefile:9650: conf/libvirt_conf_la-virdomainmomentobjlist.lo] Error 1

This fixes it:

	diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
	index 92cf52dd..2e9343ff 100644
	--- a/src/conf/virdomainmomentobjlist.c
	+++ b/src/conf/virdomainmomentobjlist.c
	@@ -165,7 +165,7 @@ virDomainMomentMoveChildren(virDomainMomentObjPtr from,
								 virDomainMomentObjPtr to)
	 {
		 virDomainMomentObjPtr child;
	-    virDomainMomentObjPtr last;
	+    virDomainMomentObjPtr last = NULL;

		 if (!from->first_child)
			 return;

> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


-- 
IBM Systems
Linux on Z & Virtualization Development
--------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--------------------------------------------------
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 3/28/19 2:40 AM, Bjoern Walk wrote:

>>>>> +/* 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.
>>>
>>> Good find from Coverity. If there are no children to move, I do need the
>>> early exit, so I'll squash that in.
>>
>> Did you forget this? Function is pushed in this (broken?) version and I
>> get a warning/error on GCC 8.0.1.
> 
> Sorry, no morning coffee yet, the fix is in, but I still get a GCC
> warning:
> 
> 	  CC       conf/libvirt_conf_la-virdomainmomentobjlist.lo
> 	../../src/conf/virdomainmomentobjlist.c: In function 'virDomainMomentMoveChildren':
> 	../../src/conf/virdomainmomentobjlist.c:178:19: error: 'last' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 		 last->sibling = to->first_child;
> 		 ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
> 	cc1: all warnings being treated as errors
> 	make[5]: *** [Makefile:9650: conf/libvirt_conf_la-virdomainmomentobjlist.lo] Error 1

So gcc isn't as smart as Coverity at seeing that it will always be
initialized.

> 
> This fixes it:
> 
> 	diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
> 	index 92cf52dd..2e9343ff 100644
> 	--- a/src/conf/virdomainmomentobjlist.c
> 	+++ b/src/conf/virdomainmomentobjlist.c
> 	@@ -165,7 +165,7 @@ virDomainMomentMoveChildren(virDomainMomentObjPtr from,
> 								 virDomainMomentObjPtr to)
> 	 {
> 		 virDomainMomentObjPtr child;
> 	-    virDomainMomentObjPtr last;
> 	+    virDomainMomentObjPtr last = NULL;

Yep, will push shortly as a build-fixer. I'm assuming you're okay if I
push it in your name, as you reported and posted the fix, even though it
wasn't the usual git format.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 3/28/19 8:51 AM, Eric Blake wrote:

> 
> So gcc isn't as smart as Coverity at seeing that it will always be
> initialized.
> 
>>
>> This fixes it:
>>
>> 	diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
>> 	index 92cf52dd..2e9343ff 100644
>> 	--- a/src/conf/virdomainmomentobjlist.c
>> 	+++ b/src/conf/virdomainmomentobjlist.c
>> 	@@ -165,7 +165,7 @@ virDomainMomentMoveChildren(virDomainMomentObjPtr from,
>> 								 virDomainMomentObjPtr to)
>> 	 {
>> 		 virDomainMomentObjPtr child;
>> 	-    virDomainMomentObjPtr last;
>> 	+    virDomainMomentObjPtr last = NULL;
> 
> Yep, will push shortly as a build-fixer. I'm assuming you're okay if I
> push it in your name, as you reported and posted the fix, even though it
> wasn't the usual git format.

Actually, I'm worried that this may trigger other warnings. I'll post a
separate message with an alternative fix proposal, and we can choose
which one looks nicer.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list