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@xxxxxxxxxx>
>> ---
>> + 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list