[libvirt] [PATCH] snapashot: Improve logic of virDomainMomentMoveChildren
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
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
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
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.
participants (3)
-
Bjoern Walk -
Eric Blake -
Ján Tomko