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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> > 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
