9:20 a.m.
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(a)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
6:12 p.m.
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(a)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
6:20 p.m.
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(a)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
7:12 a.m.
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(a)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
3964
days inactive
3973
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Michal Privoznik