[PATCH v2 1/2] qemu: process: Split out the statement to handle the qemu is allowed to reboot
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Split out the statement to handle whether the qemu is allowed to reboot or not. So that it gets available for the later patch. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_process.c | 17 +++++++++++++---- src/qemu/qemu_process.h | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3b4af61bf8..f4e67c70ad 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6360,6 +6360,18 @@ qemuProcessPrepareHostHostdevs(virDomainObj *vm) return 0; } +bool +qemuProcessRebootAllowed(const virDomainDef *def) +{ + if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { + return false; + } else { + return true; + } +} static void qemuProcessPrepareAllowReboot(virDomainObj *vm) @@ -6375,10 +6387,7 @@ qemuProcessPrepareAllowReboot(virDomainObj *vm) if (priv->allowReboot != VIR_TRISTATE_BOOL_ABSENT) return; - if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || - def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { + if (!qemuProcessRebootAllowed(def)) { priv->allowReboot = VIR_TRISTATE_BOOL_NO; } else { priv->allowReboot = VIR_TRISTATE_BOOL_YES; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 93103eb530..f9fa140e6d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -242,3 +242,5 @@ void qemuProcessQMPFree(qemuProcessQMP *proc); G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuProcessQMP, qemuProcessQMPFree); int qemuProcessQMPStart(qemuProcessQMP *proc); + +bool qemuProcessRebootAllowed(const virDomainDef *def); -- 2.27.0
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> <transient shareBacking='yes'/> option requires set-action capability if destroy action is set to all of the events configuration. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_validate.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 8906aa52d9..0d49512996 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -24,6 +24,7 @@ #include "qemu_block.h" #include "qemu_command.h" #include "qemu_domain.h" +#include "qemu_process.h" #include "domain_conf.h" #include "virlog.h" #include "virutil.h" @@ -2991,6 +2992,7 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const virDomainDiskDef *disk, static int qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, + const virDomainDef *def, virQEMUCaps *qemuCaps) { @@ -3033,6 +3035,15 @@ qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, } if (disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) { + if (!qemuProcessRebootAllowed(def) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SET_ACTION)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("transient disk backing image sharing with destroy action of lifecycle " + "isn't supported by this QEMU binary (%s)"), + disk->dst); + return -1; + } + /* sharing the backing file requires hotplug of the disk in the qemu driver */ switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_USB: @@ -3077,7 +3088,7 @@ qemuValidateDomainDeviceDefDisk(const virDomainDiskDef *disk, if (qemuValidateDomainDeviceDefDiskBlkdeviotune(disk, def) < 0) return -1; - if (qemuValidateDomainDeviceDefDiskTransient(disk, qemuCaps) < 0) + if (qemuValidateDomainDeviceDefDiskTransient(disk, def, qemuCaps) < 0) return -1; if (disk->src->shared && !disk->src->readonly && -- 2.27.0
On Mon, Aug 30, 2021 at 00:30:43 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
<transient shareBacking='yes'/> option requires set-action capability if destroy action is set to all of the events configuration.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_validate.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 8906aa52d9..0d49512996 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c
[...]
@@ -2991,6 +2992,7 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const virDomainDiskDef *disk,
static int qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, + const virDomainDef *def, virQEMUCaps *qemuCaps)
{ @@ -3033,6 +3035,15 @@ qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, }
if (disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) { + if (!qemuProcessRebootAllowed(def) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SET_ACTION)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("transient disk backing image sharing with destroy action of lifecycle " + "isn't supported by this QEMU binary (%s)"), + disk->dst); + return -1;
I didn't really consider this corner case worth worrying about. I'll exchange the two terms in the condition since I prefer when capability checks go first in such cases and put the error message on a single line for easier greppability and also add a better explanation of the specific scenario this is handling into the commit message. Series: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Mon, Aug 30, 2021 at 00:30:42 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Split out the statement to handle whether the qemu is allowed to reboot or not. So that it gets available for the later patch.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_process.c | 17 +++++++++++++---- src/qemu/qemu_process.h | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3b4af61bf8..f4e67c70ad 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6360,6 +6360,18 @@ qemuProcessPrepareHostHostdevs(virDomainObj *vm) return 0; }
+bool +qemuProcessRebootAllowed(const virDomainDef *def) +{ + if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { + return false; + } else { + return true; + } +}
if (COND) return true/false; else return false/true; is an anti-pattern. The value can be directly returned. In addition it's missing a function comment when this is actually used since you are moving it from the place which is doing the feature check.
static void qemuProcessPrepareAllowReboot(virDomainObj *vm) @@ -6375,10 +6387,7 @@ qemuProcessPrepareAllowReboot(virDomainObj *vm) if (priv->allowReboot != VIR_TRISTATE_BOOL_ABSENT) return;
- if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || - def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { + if (!qemuProcessRebootAllowed(def)) { priv->allowReboot = VIR_TRISTATE_BOOL_NO; } else { priv->allowReboot = VIR_TRISTATE_BOOL_YES;
Here we can use virTristateBoolFromBool instead of an if statement. I'll go with: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3b4af61bf8..03915f559c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6361,6 +6361,24 @@ qemuProcessPrepareHostHostdevs(virDomainObj *vm) } +/** + * qemuProcessRebootAllowed: + * @def: domain definition + * + * This function encapsulates the logic which dictated whether '-no-reboot' was + * used instead of '-no-shutdown' which is used QEMU versions which don't + * support the 'set-action' QMP command. + */ +bool +qemuProcessRebootAllowed(const virDomainDef *def) +{ + return def->onReboot != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + def->onPoweroff != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + (def->onCrash != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + def->onCrash != VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY); +} + + static void qemuProcessPrepareAllowReboot(virDomainObj *vm) { @@ -6375,14 +6393,7 @@ qemuProcessPrepareAllowReboot(virDomainObj *vm) if (priv->allowReboot != VIR_TRISTATE_BOOL_ABSENT) return; - if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || - def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { - priv->allowReboot = VIR_TRISTATE_BOOL_NO; - } else { - priv->allowReboot = VIR_TRISTATE_BOOL_YES; - } + priv->allowReboot = virTristateBoolFromBool(qemuProcessRebootAllowed(def)); } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 93103eb530..f9fa140e6d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -242,3 +242,5 @@ void qemuProcessQMPFree(qemuProcessQMP *proc); G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuProcessQMP, qemuProcessQMPFree); int qemuProcessQMPStart(qemuProcessQMP *proc); + +bool qemuProcessRebootAllowed(const virDomainDef *def);
On Tue, Aug 31, 2021 at 09:13:55AM +0200, Peter Krempa wrote:
On Mon, Aug 30, 2021 at 00:30:42 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Split out the statement to handle whether the qemu is allowed to reboot or not. So that it gets available for the later patch.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_process.c | 17 +++++++++++++---- src/qemu/qemu_process.h | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3b4af61bf8..f4e67c70ad 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6360,6 +6360,18 @@ qemuProcessPrepareHostHostdevs(virDomainObj *vm) return 0; }
+bool +qemuProcessRebootAllowed(const virDomainDef *def) +{ + if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { + return false; + } else { + return true; + } +}
if (COND) return true/false; else return false/true;
is an anti-pattern. The value can be directly returned. In addition it's missing a function comment when this is actually used since you are moving it from the place which is doing the feature check.
Thank you for pointing it out. I got it.
static void qemuProcessPrepareAllowReboot(virDomainObj *vm) @@ -6375,10 +6387,7 @@ qemuProcessPrepareAllowReboot(virDomainObj *vm) if (priv->allowReboot != VIR_TRISTATE_BOOL_ABSENT) return;
- if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || - def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { + if (!qemuProcessRebootAllowed(def)) { priv->allowReboot = VIR_TRISTATE_BOOL_NO; } else { priv->allowReboot = VIR_TRISTATE_BOOL_YES;
Here we can use virTristateBoolFromBool instead of an if statement.
I'll go with:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3b4af61bf8..03915f559c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6361,6 +6361,24 @@ qemuProcessPrepareHostHostdevs(virDomainObj *vm) }
+/** + * qemuProcessRebootAllowed: + * @def: domain definition + * + * This function encapsulates the logic which dictated whether '-no-reboot' was + * used instead of '-no-shutdown' which is used QEMU versions which don't + * support the 'set-action' QMP command. + */ +bool +qemuProcessRebootAllowed(const virDomainDef *def) +{ + return def->onReboot != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + def->onPoweroff != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || + (def->onCrash != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && + def->onCrash != VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY); +} + + static void qemuProcessPrepareAllowReboot(virDomainObj *vm) { @@ -6375,14 +6393,7 @@ qemuProcessPrepareAllowReboot(virDomainObj *vm) if (priv->allowReboot != VIR_TRISTATE_BOOL_ABSENT) return;
- if (def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY && - (def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || - def->onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY)) { - priv->allowReboot = VIR_TRISTATE_BOOL_NO; - } else { - priv->allowReboot = VIR_TRISTATE_BOOL_YES; - } + priv->allowReboot = virTristateBoolFromBool(qemuProcessRebootAllowed(def)); }
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 93103eb530..f9fa140e6d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -242,3 +242,5 @@ void qemuProcessQMPFree(qemuProcessQMP *proc); G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuProcessQMP, qemuProcessQMPFree);
int qemuProcessQMPStart(qemuProcessQMP *proc); + +bool qemuProcessRebootAllowed(const virDomainDef *def);
Thanks it's excellent! Should I post the series with your changes? - Masa
participants (2)
-
Masayoshi Mizuma -
Peter Krempa