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@xxxxxxxxxx>
> ---
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list