Re: [PATCH 03/19] virsh: Add serial/parallel device name completion to console 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-domain.c | 54 ++++++++++++++++++++++++++++++++++
>   tools/virsh-completer-domain.h |  4 +++
>   tools/virsh-domain.c           |  1 +
>   3 files changed, 59 insertions(+)
>
> diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
> index 6f67c91746..ab81a0dcfe 100644
> --- a/tools/virsh-completer-domain.c
> +++ b/tools/virsh-completer-domain.c
> @@ -617,3 +617,57 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
>       virshDomainFree(dom);
>       return ret;
>   }
> +
> +char **
> +virshDomainConsoleCompleter(vshControl *ctl,
> +                            const vshCmd *cmd,
> +                            unsigned int flags)
> +{
> +    virshControlPtr priv = ctl->privData;
> +    g_autoptr(xmlDoc) xmldoc = NULL;
> +    g_autoptr(xmlXPathContext) ctxt = NULL;
> +    int nserials, nparallels, offset = 0, head;
> +    g_autofree xmlNodePtr *serials = NULL;
> +    g_autofree xmlNodePtr *parallels = NULL;
> +    size_t i;
> +    VIR_AUTOSTRINGLIST tmp = NULL;
> +
> +    virCheckFlags(0, NULL);
> +
> +    if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
> +        return NULL;
> +
> +    if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0)
> +        return NULL;
> +
> +    nserials = virXPathNodeSet("./devices/serial", ctxt, &serials);
> +    if (nserials < 0)
> +        return NULL;
> +
> +    nparallels = virXPathNodeSet("./devices/parallel", ctxt, &parallels);
> +    if (nparallels < 0)
> +        return NULL;
> +
> +    tmp = g_new0(char *, nserials + nparallels + 1);
> +
> +    for (i = 0; i < nserials; i++) {
> +        ctxt->node = serials[i];
> +        if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
> +            offset += 1;
> +        else
> +            tmp[i - offset] = virXPathString("string(./alias/@name)", ctxt);

This is needlessly complicated way to address an item in array.

> +    }
> +
> +    head = i - offset;
> +    offset = 0;
> +
> +    for (i = 0; i < nparallels; i++) {
> +        ctxt->node = parallels[i];
> +        if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
> +            offset += 1;
> +        else
> +            tmp[head + i - offset] = virXPathString("string(./alias/@name)", ctxt);

And so it this.

> +    }
> +
> +    return g_steal_pointer(&tmp);
> +}


ACK with this squashed in:

diff --git i/tools/virsh-completer-domain.c w/tools/virsh-completer-domain.c
index ab81a0dcfe..221c891a30 100644
--- i/tools/virsh-completer-domain.c
+++ w/tools/virsh-completer-domain.c
@@ -626,10 +626,12 @@ virshDomainConsoleCompleter(vshControl *ctl,
     virshControlPtr priv = ctl->privData;
     g_autoptr(xmlDoc) xmldoc = NULL;
     g_autoptr(xmlXPathContext) ctxt = NULL;
-    int nserials, nparallels, offset = 0, head;
+    int nserials;
+    int nparallels;
     g_autofree xmlNodePtr *serials = NULL;
     g_autofree xmlNodePtr *parallels = NULL;
     size_t i;
+    size_t offset = 0;
     VIR_AUTOSTRINGLIST tmp = NULL;

     virCheckFlags(0, NULL);
@@ -652,21 +654,20 @@ virshDomainConsoleCompleter(vshControl *ctl,

     for (i = 0; i < nserials; i++) {
         ctxt->node = serials[i];
+
         if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
-            offset += 1;
-        else
-            tmp[i - offset] = virXPathString("string(./alias/@name)", 
ctxt);
+            continue;
+
+        tmp[offset++] = virXPathString("string(./alias/@name)", ctxt);
     }

-    head = i - offset;
-    offset = 0;
-
     for (i = 0; i < nparallels; i++) {
         ctxt->node = parallels[i];
+
         if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
-            offset += 1;
-        else
-            tmp[head + i - offset] = 
virXPathString("string(./alias/@name)", ctxt);
+            continue;
+
+        tmp[offset++] = virXPathString("string(./alias/@name)", ctxt);
     }

     return g_steal_pointer(&tmp);


Michal