The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 252a48b..fdfec2b 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -868,7 +868,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, graphics->data.vnc.port = port; if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error; if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) @@ -987,7 +987,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, graphics->data.vnc.port = port; if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error; if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) -- 2.4.10

On 02/10/2016 05:37 AM, Michal Privoznik wrote: I have patches on list which are similar, but I've been requested to change. In particular, this change leads to a memory leak. as the vnc needs to be deleted. John

On 02/10/2016 06:37 AM, Erik Skultety wrote: Michal asked me via IRC - the code looked eerily similar to the qemu_command code I had posted patches on which also has the same non status check. Should have more coffee in my system before diving in ;-) John

In the function, @schedMap is a bitmap used to track pinning of scheduling units (e.g. vcpu, iothreads). However, as a corner case it may happen that it's empty but the scheduler is set to either RR or FIFO. If that's the case we may get -1 when asking for the next bit set in @schedMap and proceed with that. The code is able to deal with that because we are ignoring some errors afterwards, but it's safer to check upfront. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 67415fa..a842ee4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21485,20 +21485,21 @@ virDomainFormatSchedDef(virDomainDefPtr def, case VIR_PROC_POLICY_FIFO: case VIR_PROC_POLICY_RR: virBitmapClearAll(prioMap); - hasPriority = true; /* we need to find a subset of vCPUs with the given scheduler * that share the priority */ - nextprio = virBitmapNextSetBit(schedMap, -1); - sched = func(def, nextprio); - priority = sched->priority; - - ignore_value(virBitmapSetBit(prioMap, nextprio)); - - while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { + if ((nextprio = virBitmapNextSetBit(schedMap, -1)) > -1) { sched = func(def, nextprio); - if (sched->priority == priority) - ignore_value(virBitmapSetBit(prioMap, nextprio)); + priority = sched->priority; + hasPriority = true; + + ignore_value(virBitmapSetBit(prioMap, nextprio)); + + while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { + sched = func(def, nextprio); + if (sched->priority == priority) + ignore_value(virBitmapSetBit(prioMap, nextprio)); + } } currentMap = prioMap; -- 2.4.10