On 01/09/2014 05:12 PM, Eric Blake wrote:
On 01/07/2014 08:20 AM, Michal Privoznik wrote:
Currently, the @flags usage is a bit unclear at first sight to say the least. There's no need for such unclear code especially when we can borrow the working code from qemuDomainShutdownFlags().
Working, but awkward to read. I think the qemu code also could benefit from the rewrite I propose below.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
+ if (signalRequested || rc == 0) {
On the left side of ||, the condition is true for A, C, and D - which is wrong if initctl succeeded [ouch]. On the right side of the ||, the condition is always true for C, as well as sometimes true for A and D (depending on whether initctl succeeded).
Simplifying this to 'if (rc == 0)' would fix the broken logic.
But would still be hard to read.
Still broken; needs a v3
May I suggest this layout: initctlRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_INITCTL; signalRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_SIGNAL; if (initctlRequested) { rc = attempt; if (rc < 0) goto error; if (rc > 0) goto success; // didn't work, fall through to next attempt } if (signalRequested) { rc = attempt; if (rc < 0) goto error; if (rc > 0) goto success; // didn't work, fall through to next attempt } // since we got here, nothing requested worked, so goto error; and quit mentioning signalRequested inside the initctlRequested block, as well as quit trying to use || on the conditions of whether to attempt a signal. Using a 'goto success' at the point where we know it worked, instead of trying to write magic conditions to avoid duplicate work, is easier to read. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org