snapashot: Improve logic of virDomainMomentMoveChildren

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

Posted by Eric Blake 6 years, 10 months ago

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.

Fixes: ced0898f86bf
Reported-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Eric Blake <eblake@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;
-    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;
     from->nchildren = 0;
     from->first_child = NULL;
-- 
2.20.1

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

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

Posted by Ján Tomko 6 years, 10 months ago

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@linux.ibm.com>
>Signed-off-by: Eric Blake <eblake@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@redhat.com>

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

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

Posted by Eric Blake 6 years, 10 months ago

On 3/28/19 10:18 AM, Ján Tomko wrote:
> s/snapashot/snapshot/
> 

I've been making that one a lot; will fix.

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

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

Oh, good point. I'll keep the early exit for (!from->nchildren) then,

> But the callers are reasonable and only call this if (from->nchildren)

because I'm not certain that all future callers will be reasonable.

> 
>>     from->nchildren = 0;
>>     from->first_child = NULL;
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>

I'll go ahead and push this one with those fixes.

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

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

Posted by Bjoern Walk 6 years, 10 months ago

Eric Blake <eblake@redhat.com> [2019-03-28, 09:05AM -0500]:
> 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.
> 
> Fixes: ced0898f86bf
> Reported-by: Bjoern Walk <bwalk@linux.ibm.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Qualifies as a build-breaker fix, but I'd like a review before pushing.

Looks good to me.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list