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