Re: [PATCH] lxcDomainShutdownFlags: Cleanup @flags usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 27.12.2013 21:35, Eric Blake wrote:
> 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@xxxxxxxxxx>
>>> ---
>>>  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
> 

Yep, I've pushed it accidentally. I've merged a branch which wasn't
rebased onto master but onto a different local brach containing this
commit. I suggest reverting my commit. I'll post better version (with
lxcDomainReboot) then. Sorry for the noise.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]