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

[Masayoshi Mizuma]

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 at 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 at 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