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

[Peter Krempa <pkrempa@xxxxxxxxxx>]

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.


>  
>  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);