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@xxxxxxxxxx>
> ---
>
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list