[PATCH 07/28] vsh: Introduce tool to find unwanted positional arguments to 'self-test'

[Peter Krempa <pkrempa@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
 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;
         }
     }
-- 
2.44.0
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx