[libvirt] [PATCH] lxcDomainShutdownFlags: Cleanup @flags usage

[Eric Blake]

On 12/19/2013 09:15 AM, Daniel P. Berrange wrote:
> On Wed, Dec 18, 2013 at 07:19:31PM +0100, 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().
>>
>> In addition, this fixes one bug too. If user requested both
>> VIR_DOMAIN_SHUTDOWN_INITCTL and VIR_DOMAIN_SHUTDOWN_SIGNAL at the same
>> time, he is basically saying: 'Use the force Luke! If initctl fails try
>> sending a signal.' But with the current code we don't do that. If
>> initctl fails for some reason (e.g. inability to write to /dev/initctl)
>> we don't try sending any signal but fail immediately. To make things
>> worse, making a domain shutdown with bare _SIGNAL was working by blind
>> chance of a @rc variable being placed at correct place on the stack so
>> its initial value was zero.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/lxc/lxc_driver.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
>> index c499182..a563b70 100644
>> --- a/src/lxc/lxc_driver.c
>> +++ b/src/lxc/lxc_driver.c
>> @@ -2594,7 +2594,8 @@ lxcDomainShutdownFlags(virDomainPtr dom,
>>      virDomainObjPtr vm;
>>      char *vroot = NULL;
>>      int ret = -1;
>> -    int rc;
>> +    int rc = 0;
>> +    bool useInitctl = false, initctlRequested, signalRequested;
>>  
>>      virCheckFlags(VIR_DOMAIN_SHUTDOWN_INITCTL |
>>                    VIR_DOMAIN_SHUTDOWN_SIGNAL, -1);
>> @@ -2623,25 +2624,24 @@ lxcDomainShutdownFlags(virDomainPtr dom,
>>                      (unsigned long long)priv->initpid) < 0)
>>          goto cleanup;
>>  
>> -    if (flags == 0 ||
>> -        (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) {
>> -        if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF,
>> -                                        vroot)) < 0) {
>> +    initctlRequested = flags & VIR_DOMAIN_SHUTDOWN_INITCTL;
>> +    signalRequested = flags & VIR_DOMAIN_SHUTDOWN_SIGNAL;
>> +
>> +    if (initctlRequested || !flags)
>> +        useInitctl = true;
> 
> I'm not sure this logic is right.

Yuck - commit aa4619337 is broken.

> 
> I think we want to do without useInitctl and have
> 
>     initctlRequested = !flags || (flags & VIR_DOMAIN_SHUTDOWN_INITCTL);
>     signalRequested = !flags || (flags & VIR_DOMAIN_SHUTDOWN_SIGNAL);
> 
> because otherwise
> 
>> +
>> +    if (useInitctl) {
>> +        rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, vroot);
>> +        if (rc < 0 && !signalRequested)
>>              goto cleanup;
>> -        }
>> -        if (rc == 0 && flags != 0 &&
>> -            ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
>> +        if (rc == 0 && !signalRequested) {
> 
> This code block won't fallback to trying the signal method
> when flags == 0

This is still unfortunately the case in what you pushed.  And how come
you didn't do the same treatment to lxcDomainReboot, which also has two
different flags where we need proper fallback when initctl is
unsupported?  Your commit conflicts with the pending CVE-2013-6456
efforts (my patch at [1]), so we need to get this resolved.

[1] https://www.redhat.com/archives/libvir-list/2013-December/msg01249.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131227/3adad048/attachment-0001.sig>