Re: [PATCH v2 1/2] qemu: process: Split out the statement to handle the qemu is allowed to reboot

On Mon, Aug 30, 2021 at 00:30:42 -0400, Masayoshi Mizuma wrote: > From: Masayoshi Mizuma <m.mizuma(a)jp.fujitsu.com&gt; > > 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(a)jp.fujitsu.com&gt; > --- > 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);