[libvirt] [PATCH v2] lxcDomainShutdownFlags and lxcDomainReboot: 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(). Signed-off-by: Michal Privoznik <mprivozn@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; 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; + signalRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_SIGNAL; + + if (initctlRequested) { + 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 (signalRequested || rc == 0) { if (kill(priv->initpid, SIGTERM) < 0 && errno != ESRCH) { virReportSystemError(errno, @@ -2781,7 +2778,8 @@ lxcDomainReboot(virDomainPtr dom, virDomainObjPtr vm; char *vroot = NULL; int ret = -1; - int rc; + int rc = 0; + bool initctlRequested, signalRequested; virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL | VIR_DOMAIN_REBOOT_SIGNAL, -1); @@ -2810,25 +2808,21 @@ lxcDomainReboot(virDomainPtr dom, (unsigned long long)priv->initpid) < 0) goto cleanup; - if (flags == 0 || - (flags & VIR_DOMAIN_REBOOT_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, - vroot)) < 0) { + initctlRequested = !flags || flags & VIR_DOMAIN_REBOOT_INITCTL; + signalRequested = !flags || flags & VIR_DOMAIN_REBOOT_SIGNAL; + + if (initctlRequested) { + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, vroot); + if (rc < 0) 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_REBOOT_SIGNAL))) { + if (signalRequested || rc == 0) { if (kill(priv->initpid, SIGHUP) < 0 && errno != ESRCH) { virReportSystemError(errno, -- 1.8.5.1
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@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
On 01/09/2014 05:12 PM, Eric Blake wrote:
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().
Working, but awkward to read. I think the qemu code also could benefit from the rewrite I propose below.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
+ 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.
But would still be hard to read.
Still broken; needs a v3
May I suggest this layout: initctlRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_INITCTL; signalRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_SIGNAL; if (initctlRequested) { rc = attempt; if (rc < 0) goto error; if (rc > 0) goto success; // didn't work, fall through to next attempt } if (signalRequested) { rc = attempt; if (rc < 0) goto error; if (rc > 0) goto success; // didn't work, fall through to next attempt } // since we got here, nothing requested worked, so goto error; and quit mentioning signalRequested inside the initctlRequested block, as well as quit trying to use || on the conditions of whether to attempt a signal. Using a 'goto success' at the point where we know it worked, instead of trying to write magic conditions to avoid duplicate work, is easier to read. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10.01.2014 01:20, Eric Blake wrote:
On 01/09/2014 05:12 PM, Eric Blake wrote:
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().
Working, but awkward to read. I think the qemu code also could benefit from the rewrite I propose below.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
+ 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.
But would still be hard to read.
Still broken; needs a v3
May I suggest this layout:
initctlRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_INITCTL; signalRequested = !flags || flags & VIR_DOMAIN_SHUTDOWN_SIGNAL;
if (initctlRequested) { rc = attempt; if (rc < 0) goto error;
I don't think so. If flags == 0 then we should try both initctl and signal. But with this pattern we will not in case when initctl fails. With current code (and my approach) if initctl fails we can still use the signal. Michal
participants (2)
-
Eric Blake -
Michal Privoznik