Re: [PATCH 02/19] virsh: Add logical CPU IDs completion for nodecpustats command

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 11/2/20 9:26 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@xxxxxxxx>
> ---
>   tools/virsh-completer-host.c | 35 +++++++++++++++++++++++++++++++++++
>   tools/virsh-completer-host.h |  4 ++++
>   tools/virsh-host.c           |  1 +
>   3 files changed, 40 insertions(+)
>
> diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c
> index 339390aa00..a74578d2e2 100644
> --- a/tools/virsh-completer-host.c
> +++ b/tools/virsh-completer-host.c
> @@ -136,3 +136,38 @@ virshCellnoCompleter(vshControl *ctl,
>   
>       return g_steal_pointer(&tmp);
>   }
> +
> +
> +char **
> +virshCpuCompleter(vshControl *ctl,
> +                  const vshCmd *cmd G_GNUC_UNUSED,
> +                  unsigned int flags)
> +{
> +    int i, cpuid = 0, cpunum, offset = 0;
> +    unsigned int online;
> +    g_autofree unsigned char *cpumap = NULL;
> +    char **ret = NULL;
> +    VIR_AUTOSTRINGLIST tmp = NULL;
> +    virshControlPtr priv = ctl->privData;
> +
> +    virCheckFlags(0, NULL);
> +
> +    if ((cpunum = virNodeGetCPUMap(priv->conn, &cpumap, &online, 0)) < 0)
> +        return NULL;
> +
> +    tmp = g_new0(char *, online + 1);
> +
> +    for (i = 0; i < cpunum; i++) {
> +        if (VIR_CPU_USED(cpumap, cpuid) == 0) {
> +            offset += 1;
> +            cpuid += 1;
> +            continue;
> +        } else {
> +            tmp[i - offset] = g_strdup_printf("%u", cpuid++);
> +        }
> +    }

This loop is needlessly complicated.

> +
> +    ret = g_steal_pointer(&tmp);

@ret can be dropped.
> +
> +    return ret;
> +}

ACK with this squashed in:

diff --git i/tools/virsh-completer-host.c w/tools/virsh-completer-host.c
index a74578d2e2..685fa23fd4 100644
--- i/tools/virsh-completer-host.c
+++ w/tools/virsh-completer-host.c
@@ -143,12 +143,13 @@ virshCpuCompleter(vshControl *ctl,
                   const vshCmd *cmd G_GNUC_UNUSED,
                   unsigned int flags)
 {
-    int i, cpuid = 0, cpunum, offset = 0;
+    virshControlPtr priv = ctl->privData;
+    VIR_AUTOSTRINGLIST tmp = NULL;
+    size_t i;
+    int cpunum;
+    size_t offset = 0;
     unsigned int online;
     g_autofree unsigned char *cpumap = NULL;
-    char **ret = NULL;
-    VIR_AUTOSTRINGLIST tmp = NULL;
-    virshControlPtr priv = ctl->privData;

     virCheckFlags(0, NULL);

@@ -158,16 +159,11 @@ virshCpuCompleter(vshControl *ctl,
     tmp = g_new0(char *, online + 1);

     for (i = 0; i < cpunum; i++) {
-        if (VIR_CPU_USED(cpumap, cpuid) == 0) {
-            offset += 1;
-            cpuid += 1;
+        if (VIR_CPU_USED(cpumap, i) == 0)
             continue;
-        } else {
-            tmp[i - offset] = g_strdup_printf("%u", cpuid++);
-        }
+
+        tmp[offset++] = g_strdup_printf("%zu", i);
     }

-    ret = g_steal_pointer(&tmp);
-
-    return ret;
+    return g_steal_pointer(&tmp);
 }


Michal