[libvirt] [PATCH] lxcDomainShutdownFlags: Cleanup @flags usage
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@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; + + 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) { 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, -- 1.8.5.1
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@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 :|
On 12/19/2013 09:15 AM, Daniel P. Berrange wrote:
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@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.
Yuck - commit aa4619337 is broken.
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
This is still unfortunately the case in what you pushed. And how come you didn't do the same treatment to lxcDomainReboot, which also has two different flags where we need proper fallback when initctl is unsupported? Your commit conflicts with the pending CVE-2013-6456 efforts (my patch at [1]), so we need to get this resolved. [1] https://www.redhat.com/archives/libvir-list/2013-December/msg01249.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 27.12.2013 21:35, Eric Blake wrote:
On 12/19/2013 09:15 AM, Daniel P. Berrange wrote:
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@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.
Yuck - commit aa4619337 is broken.
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
This is still unfortunately the case in what you pushed. And how come you didn't do the same treatment to lxcDomainReboot, which also has two different flags where we need proper fallback when initctl is unsupported? Your commit conflicts with the pending CVE-2013-6456 efforts (my patch at [1]), so we need to get this resolved.
[1] https://www.redhat.com/archives/libvir-list/2013-December/msg01249.html
Yep, I've pushed it accidentally. I've merged a branch which wasn't rebased onto master but onto a different local brach containing this commit. I suggest reverting my commit. I'll post better version (with lxcDomainReboot) then. Sorry for the noise. Michal
On 01/06/2014 03:18 AM, Michal Privoznik wrote:
+ + if (initctlRequested || !flags) + useInitctl = true;
I'm not sure this logic is right.
Yuck - commit aa4619337 is broken.
This is still unfortunately the case in what you pushed. And how come you didn't do the same treatment to lxcDomainReboot, which also has two different flags where we need proper fallback when initctl is unsupported? Your commit conflicts with the pending CVE-2013-6456 efforts (my patch at [1]), so we need to get this resolved.
[1] https://www.redhat.com/archives/libvir-list/2013-December/msg01249.html
Yep, I've pushed it accidentally. I've merged a branch which wasn't rebased onto master but onto a different local brach containing this commit. I suggest reverting my commit. I'll post better version (with lxcDomainReboot) then. Sorry for the noise.
No problems; we still have time to get it right before the release. For now, I've pushed the revert patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik