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(a)redhat.com&gt; --- 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;

+ + 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) {

virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("Container does not provide an initctl pipe")); goto cleanup; } - } else { - rc = 0; } - if (rc == 0 && - (flags == 0 || - (flags & VIR_DOMAIN_SHUTDOWN_SIGNAL))) { + if (rc == 0 && !useInitctl) { if (kill(priv->initpid, SIGTERM) < 0 && errno != ESRCH) { virReportSystemError(errno,

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(a)redhat.com&gt; >> --- >> 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