Re: [PATCH 04/12] virsh: Use VIR_ENUM_* for --target argument in cmdDomPMSuspend

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 6/15/21 2:38 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@xxxxxxxx>
> ---
>  tools/virsh-domain.c | 10 ++--------
>  tools/virsh-domain.h |  1 +
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 0100652e76..3fb37ea5e6 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3472,7 +3472,7 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd)
>      const char *name;
>      bool ret = false;
>      const char *target = NULL;
> -    unsigned int suspendTarget;
> +    int suspendTarget;
>      unsigned long long duration = 0;
>
>      if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
> @@ -3484,13 +3484,7 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd)
>      if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0)
>          goto cleanup;
>
> -    if (STREQ(target, "mem")) {
> -        suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM;
> -    } else if (STREQ(target, "disk")) {
> -        suspendTarget = VIR_NODE_SUSPEND_TARGET_DISK;
> -    } else if (STREQ(target, "hybrid")) {
> -        suspendTarget = VIR_NODE_SUSPEND_TARGET_HYBRID;
> -    } else {
> +    if ((suspendTarget = virNodeSuspendTargetTypeFromString(target)) < 0) {
>          vshError(ctl, "%s", _("Invalid target"));
>          goto cleanup;
>      }
> diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h
> index 0c1cc7a630..b569dd8d91 100644
> --- a/tools/virsh-domain.h
> +++ b/tools/virsh-domain.h
> @@ -44,3 +44,4 @@ VIR_ENUM_DECL(virDomainProcessSignal);
>  VIR_ENUM_DECL(virDomainLifecycle);
>  VIR_ENUM_DECL(virDomainLifecycleAction);
>  VIR_ENUM_DECL(virDomainCoreDumpFormat);
> +VIR_ENUM_DECL(virNodeSuspendTarget);
>

This doesn't feel right. I understand why you added it - so that you can
use virNodeSuspendTargetTypeFromString() in the hunk above. However, the
enum is implemented and declared in virsh-host files. Thus, what this
patch should instead is include virsh-host.h from virsh-domain.c. IOW,
I'm squashing in the following:

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 3fb37ea5e6..de80b1c2d5 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -43,6 +43,7 @@
 #include "virstring.h"
 #include "virsh-console.h"
 #include "virsh-domain-monitor.h"
+#include "virsh-host.h"
 #include "virerror.h"
 #include "virtime.h"
 #include "virtypedparam.h"
diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h
index b569dd8d91..0c1cc7a630 100644
--- a/tools/virsh-domain.h
+++ b/tools/virsh-domain.h
@@ -44,4 +44,3 @@ VIR_ENUM_DECL(virDomainProcessSignal);
 VIR_ENUM_DECL(virDomainLifecycle);
 VIR_ENUM_DECL(virDomainLifecycleAction);
 VIR_ENUM_DECL(virDomainCoreDumpFormat);
-VIR_ENUM_DECL(virNodeSuspendTarget);

Michal