[libvirt] [PATCH] snapashot: Improve logic of virDomainMomentMoveChildren

[Ján Tomko]

s/snapashot/snapshot/

On Thu, Mar 28, 2019 at 09:05:31AM -0500, Eric Blake wrote:
>Even though Coverity can prove that 'last' is always set if the prior
>loop executed, gcc 8.0.1 cannot:
>
>  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;
>         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
>
>Rewrite the loop to a form that should be easier for static analysis
>to work with.
>

The build works for me so I cannot judge that part

>Fixes: ced0898f86bf
>Reported-by: Bjoern Walk <bwalk at linux.ibm.com>
>Signed-off-by: Eric Blake <eblake at redhat.com>
>---
>
>Qualifies as a build-breaker fix, but I'd like a review before pushing.
>
> src/conf/virdomainmomentobjlist.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
>diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
>index b9ca5b1318..5a217056d1 100644
>--- a/src/conf/virdomainmomentobjlist.c
>+++ b/src/conf/virdomainmomentobjlist.c
>@@ -164,18 +164,17 @@ void
> virDomainMomentMoveChildren(virDomainMomentObjPtr from,
>                             virDomainMomentObjPtr to)
> {
>-    virDomainMomentObjPtr child;
>-    virDomainMomentObjPtr last;
>+    virDomainMomentObjPtr child = from->first_child;
>
>-    if (!from->first_child)
>-        return;

From the code-change point-of view, by removing this condition,

>-    for (child = from->first_child; child; child = child->sibling) {
>+    while (child) {
>         child->parent = to;
>-        if (!child->sibling)
>-            last = child;
>+        if (!child->sibling) {
>+            child->sibling = to->first_child;
>+            break;
>+        }
>+        child = child->sibling;
>     }
>     to->nchildren += from->nchildren;
>-    last->sibling = to->first_child;
>     to->first_child = from->first_child;

this possibly erases 'to->first_child' if 'from' does not have any.
But the callers are reasonable and only call this if (from->nchildren)

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

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190328/b7f21b1b/attachment-0001.sig>