[libvirt] [PATCH v2] lxcDomainShutdownFlags and lxcDomainReboot: Cleanup @flags usage

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140109/7f2beb7f/attachment-0001.sig>