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().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
diff to v1: -lxcDomainReboot adjusted too -drop @useInitctl -shorten the commit message a bit
src/lxc/lxc_driver.c | 46 ++++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 26 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7e56a59..06263b9 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2701,7 +2701,8 @@ lxcDomainShutdownFlags(virDomainPtr dom, virDomainObjPtr vm; char *vroot = NULL; int ret = -1; - int rc; + int rc = 0; + bool initctlRequested, signalRequested;
I'm going to track all four scenarios: A. flags == 0 B. flags == INITCTL C. flags == SIGNAL D. flags == INITCTL | SIGNAL
virCheckFlags(VIR_DOMAIN_SHUTDOWN_INITCTL | VIR_DOMAIN_SHUTDOWN_SIGNAL, -1); @@ -2730,25 +2731,21 @@ 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 || flags & VIR_DOMAIN_SHUTDOWN_INITCTL;
That is, A, B, and D now have initctlRequested true.
+ signalRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_SIGNAL;
That is, A, C, and D now have signalRequested true.
+ + if (initctlRequested) { + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, vroot); + if (rc < 0 && !signalRequested) goto cleanup;
This says if we have a fatal error while trying initctl, then in scenario B we go to cleanup, otherwise (A or D) we see what else we can do. I'm wondering if we should unconditionally goto cleanup on 'rc < 0', since that implies a pretty major failure in the framework (such as a failed syscall), but I suppose ignoring the error and trying signal anyways also works.
- } - 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;
Only get here for condition B, at which point the error is correct - we can't shut down the guest by the only means requested.
} - } else { - rc = 0; }
At this point, rc is either 0 (condition C - we skipped the 'if') or the result of our initctl attempt (< 0 for fatal, 0 for unsupported, 1 for succeeded - but rc <=0 is limited to condition A or D since we already went to cleanup on condition B).
- if (rc == 0 && - (flags == 0 || - (flags & VIR_DOMAIN_SHUTDOWN_SIGNAL))) { + 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. Still broken; needs a v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org