From 348010ac937fd9a71d81cd3d4154dd46bdbb6a87 Mon Sep 17 00:00:00 2001
From: Peter Krempa <pkrempa@redhat.com>
Date: Wed, 13 Mar 2024 21:13:44 +0100
Subject: [PATCH] vsh: Introduce tool to find unwanted positional arguments to
 'self-test'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

While the virsh option definitions specify (either explicitly after
recent refactors, or implicitly before) whether an argument is
positional or not, the actual parser is way more lax and actually and
allows also arguments which were considered/documented as non-positional
to be filled positionally unless VSH_OFLAG_REQ_OPT is used in the flags.

This creates situations such as 'snapshot-create-as' which has the
following docs:

SYNOPSIS
  snapshot-create-as <domain> [--name <string>] [--description <string>]
    [--print-xml] [--no-metadata] [--halt] [--disk-only]
    [--reuse-external] [--quiesce] [--atomic] [--live] [--validate]
    [--memspec <string>] [[--diskspec] <string>]...

Thus showing as if '--name' and '--description' required the option, but
in fact the following happens when only positionals are passed:

  $ virsh snapshot-create-as --print-xml 1 2 3 4 5
  <domainsnapshot>
    <name>2</name>
    <description>3</description>
    <disks>
      <disk name='4'/>
      <disk name='5'/>
    </disks>
  </domainsnapshot>

In the above example e.g. '--memspec' is not populated.

This disconnect makes it impossible to refactor the parser itself and
allows users to write buggy interactions with virsh.

In order to address this we'll be annotating every single of these
unwanted positional options as such so that this doesn't happen in the
future, while still preserving the quirk in the parser.

This patch introduces a tool which outputs list of options which are not
marked as positional but are lacking the VSH_OFLAG_REQ_OPT flag.

This tool will be removed once all the offenders found by it will be
addressed.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 tools/vsh.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index a4235905dc..96d6a5c238 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -244,11 +244,13 @@ static int disconnected; /* we may have been disconnected */
 static int
 vshCmddefCheckInternals(vshControl *ctl,
                         const vshCmdDef *cmd,
-                        bool missingCompleters)
+                        bool missingCompleters,
+                        int brokenPositionals)
 {
     size_t i;
     bool seenOptionalOption = false;
     g_auto(virBuffer) complbuf = VIR_BUFFER_INITIALIZER;
+    g_auto(virBuffer) posbuf = VIR_BUFFER_INITIALIZER;
 
     /* in order to perform the validation resolve the alias first */
     if (cmd->alias) {
@@ -325,6 +327,28 @@ vshCmddefCheckInternals(vshControl *ctl,
             }
         }
 
+        if (brokenPositionals >= 0) {
+            switch (opt->type) {
+            case VSH_OT_INT:
+            case VSH_OT_STRING:
+            case VSH_OT_ARGV:
+                if (brokenPositionals == 0 ||
+                    brokenPositionals == opt->type) {
+                    if (!(opt->flags & VSH_OFLAG_REQ_OPT) && !opt->positional)
+                        virBufferStrcat(&posbuf, opt->name, ", ", NULL);
+                }
+                break;
+
+            case VSH_OT_BOOL:
+                /* only name is completed */
+                /* no point in completing numbers */
+            case VSH_OT_ALIAS:
+                /* alias is handled in the referenced command */
+            case VSH_OT_NONE:
+                break;
+            }
+        }
+
         /* require that positional non-argv options are required */
         if (opt->positional && !opt->required && opt->type != VSH_OT_ARGV) {
             vshError(ctl, "positional argument '%s' of command '%s' must be required",
@@ -427,10 +451,15 @@ vshCmddefCheckInternals(vshControl *ctl,
     }
 
     virBufferTrim(&complbuf, ", ");
+    virBufferTrim(&posbuf, ", ");
 
     if (missingCompleters && virBufferUse(&complbuf) > 0)
         vshPrintExtra(ctl, "%s: %s\n", cmd->name, virBufferCurrentContent(&complbuf));
 
+    if (virBufferUse(&posbuf)) {
+        vshPrintExtra(ctl, "%s: %s\n", cmd->name, virBufferCurrentContent(&posbuf));
+    }
+
     return 0;
 }
 
@@ -3336,6 +3365,11 @@ const vshCmdOptDef opts_selftest[] = {
      .type = VSH_OT_BOOL,
      .help = N_("output help for each command")
     },
+    {.name = "broken-positionals",
+     .type = VSH_OT_INT,
+     .flags = VSH_OFLAG_REQ_OPT,
+     .help = N_("debug positional args")
+    },
     {.name = NULL}
 };
 const vshCmdInfo info_selftest = {
@@ -3350,6 +3384,9 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd)
     const vshCmdDef *def;
     bool completers = vshCommandOptBool(cmd, "completers-missing");
     bool dumphelp = vshCommandOptBool(cmd, "dump-help");
+    int brokenPositionals = -1;
+
+    ignore_value(vshCommandOptInt(ctl, cmd, "broken-positionals", &brokenPositionals));
 
     for (grp = cmdGroups; grp->name; grp++) {
         for (def = grp->commands; def->name; def++) {
@@ -3357,7 +3394,7 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd)
             if (dumphelp && !def->alias)
                 vshCmddefHelp(def);
 
-            if (vshCmddefCheckInternals(ctl, def, completers) < 0)
+            if (vshCmddefCheckInternals(ctl, def, completers, brokenPositionals) < 0)
                 return false;
         }
     }