On 10.01.2014 01:20, Eric Blake wrote:
> 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;
I don't think so. If flags == 0 then we should try both initctl and
signal. But with this pattern we will not in case when initctl fails.
With current code (and my approach) if initctl fails we can still use
the signal.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list