[libvirt] [PATCH] lxcDomainShutdownFlags: Cleanup @flags usage

[Daniel P. Berrange]

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 at redhat.com>
> ---
>  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.

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

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


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|