[PATCH 2/2] virsh: fix regression in parsing optional integer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Regression introduced in 0.8.5, commit c1564268.  The command
'virsh freecell 0' quit working when it changed from an optional
string to an optional integer.

This patch introduces a slight change that specifying an option
twice is now detected as an error.

* tools/virsh.c (vshCmddefGetData, vshCmddefGetOption)
(vshCommandCheckOpts): Alter parameters to use bitmaps.
(vshCmddefOptParse): New function.
(vshCommandParse): Update for better handling of positional
arguments.
(vshCmddefHelp): Allow unit tests to validate options.
---
 tools/virsh.c |  149 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 104 insertions(+), 45 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index cd1d246..486442e 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -57,6 +57,7 @@
 #include "configmake.h"
 #include "threads.h"
 #include "command.h"
+#include "count-one-bits.h"

 static char *progname;

@@ -10930,66 +10931,107 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name)
     return NULL;
 }

+static int
+vshCmddefOptParse(const vshCmdDef *cmd, uint32_t* opts_need_arg,
+                  uint32_t *opts_required)
+{
+    int i;
+    bool optional = false;
+
+    if (!cmd->opts)
+        return 0;
+
+    for (i = 0; cmd->opts[i].name; i++) {
+        const vshCmdOptDef *opt = &cmd->opts[i];
+
+        if (i > 31)
+            return -1; /* too many options */
+        if (opt->type == VSH_OT_BOOL) {
+            if (opt->flag & VSH_OFLAG_REQ)
+                return -1; /* bool options can't be mandatory */
+            continue;
+        }
+        *opts_need_arg |= 1 << i;
+        if (opt->flag & VSH_OFLAG_REQ) {
+            if (optional)
+                return -1; /* mandatory options must be listed first */
+            *opts_required |= 1 << i;
+        } else {
+            optional = true;
+        }
+    }
+    return 0;
+}
+
 static const vshCmdOptDef *
-vshCmddefGetOption(const vshCmdDef * cmd, const char *name)
+vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name,
+                   uint32_t *opts_seen)
 {
-    const vshCmdOptDef *opt;
+    int i;

-    for (opt = cmd->opts; opt && opt->name; opt++)
-        if (STREQ(opt->name, name))
+    for (i = 0; cmd->opts && cmd->opts[i].name; i++) {
+        const vshCmdOptDef *opt = &cmd->opts[i];
+
+        if (STREQ(opt->name, name)) {
+            if (*opts_seen & (1 << i)) {
+                vshError(ctl, _("option --%s already seen"), name);
+                return NULL;
+            }
+            *opts_seen |= 1 << i;
             return opt;
+        }
+    }
+
+    vshError(ctl, _("command '%s' doesn't support option --%s"),
+             cmd->name, name);
     return NULL;
 }

 static const vshCmdOptDef *
-vshCmddefGetData(const vshCmdDef * cmd, int data_ct)
+vshCmddefGetData(const vshCmdDef *cmd, uint32_t *opts_need_arg,
+                 uint32_t *opts_seen)
 {
+    int i;
     const vshCmdOptDef *opt;

-    for (opt = cmd->opts; opt && opt->name; opt++) {
-        if (opt->type >= VSH_OT_DATA ||
-            (opt->type == VSH_OT_INT && (opt->flag & VSH_OFLAG_REQ))) {
-            if (data_ct == 0 || opt->type == VSH_OT_ARGV)
-                return opt;
-            else
-                data_ct--;
-        }
-    }
-    return NULL;
+    if (!*opts_need_arg)
+        return NULL;
+
+    /* Grab least-significant set bit */
+    i = count_one_bits(*opts_need_arg ^ (*opts_need_arg - 1)) - 1;
+    opt = &cmd->opts[i];
+    if (opt->type != VSH_OT_ARGV)
+        *opts_need_arg &= ~(1 << i);
+    *opts_seen |= 1 << i;
+    return opt;
 }

 /*
  * Checks for required options
  */
 static int
-vshCommandCheckOpts(vshControl *ctl, const vshCmd *cmd)
+vshCommandCheckOpts(vshControl *ctl, const vshCmd *cmd, uint32_t opts_required,
+                    uint32_t opts_seen)
 {
     const vshCmdDef *def = cmd->def;
-    const vshCmdOptDef *d;
-    int err = 0;
-
-    for (d = def->opts; d && d->name; d++) {
-        if (d->flag & VSH_OFLAG_REQ) {
-            vshCmdOpt *o = cmd->opts;
-            int ok = 0;
-
-            while (o && ok == 0) {
-                if (o->def == d)
-                    ok = 1;
-                o = o->next;
-            }
-            if (!ok) {
-                vshError(ctl,
-                         d->type == VSH_OT_DATA ?
-                         _("command '%s' requires <%s> option") :
-                         _("command '%s' requires --%s option"),
-                         def->name, d->name);
-                err = 1;
-            }
+    int i;
+
+    opts_required &= ~opts_seen;
+    if (!opts_required)
+        return 0;

+    for (i = 0; def->opts[i].name; i++) {
+        if (opts_required & (1 << i)) {
+            const vshCmdOptDef *opt = &def->opts[i];
+
+            vshError(ctl,
+                     opt->type == VSH_OT_DATA ?
+                     _("command '%s' requires <%s> option") :
+                     _("command '%s' requires --%s option"),
+                     def->name, opt->name);
         }
     }
-    return !err;
+    return -1;
 }

 static const vshCmdDef *
@@ -11055,6 +11097,14 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
         const char *desc = _(vshCmddefGetInfo(def, "desc"));
         const char *help = _(vshCmddefGetInfo(def, "help"));
         char buf[256];
+        uint32_t opts_need_arg;
+        uint32_t opts_required;
+
+        if (vshCmddefOptParse(def, &opts_need_arg, &opts_required)) {
+            vshError(ctl, _("internal error: bad options in command: '%s'"),
+                     def->name);
+            return FALSE;
+        }

         fputs(_("  NAME\n"), stdout);
         fprintf(stdout, "    %s - %s\n", def->name, help);
@@ -11731,7 +11781,9 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
         const vshCmdDef *cmd = NULL;
         vshCommandToken tk;
         bool data_only = false;
-        int data_ct = 0;
+        uint32_t opts_need_arg = 0;
+        uint32_t opts_required = 0;
+        uint32_t opts_seen = 0;

         first = NULL;

@@ -11754,6 +11806,13 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
                     vshError(ctl, _("unknown command: '%s'"), tkdata);
                     goto syntaxError;   /* ... or ignore this command only? */
                 }
+                if (vshCmddefOptParse(cmd, &opts_need_arg,
+                                      &opts_required) < 0) {
+                    vshError(ctl,
+                             _("internal error: bad options in command: '%s'"),
+                             tkdata);
+                    goto syntaxError;
+                }
                 VIR_FREE(tkdata);
             } else if (data_only) {
                 goto get_data;
@@ -11764,10 +11823,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
                     *optstr = '\0'; /* convert the '=' to '\0' */
                     optstr = vshStrdup(ctl, optstr + 1);
                 }
-                if (!(opt = vshCmddefGetOption(cmd, tkdata + 2))) {
-                    vshError(ctl,
-                             _("command '%s' doesn't support option --%s"),
-                             cmd->name, tkdata + 2);
+                if (!(opt = vshCmddefGetOption(ctl, cmd, tkdata + 2,
+                                               &opts_seen))) {
                     VIR_FREE(optstr);
                     goto syntaxError;
                 }
@@ -11789,6 +11846,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
                                  VSH_OT_INT ? _("number") : _("string"));
                         goto syntaxError;
                     }
+                    opts_need_arg &= ~opts_seen;
                 } else {
                     tkdata = NULL;
                     if (optstr) {
@@ -11804,7 +11862,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
                 continue;
             } else {
 get_data:
-                if (!(opt = vshCmddefGetData(cmd, data_ct++))) {
+                if (!(opt = vshCmddefGetData(cmd, &opts_need_arg,
+                                             &opts_seen))) {
                     vshError(ctl, _("unexpected data '%s'"), tkdata);
                     goto syntaxError;
                 }
@@ -11840,7 +11899,7 @@ get_data:
             c->def = cmd;
             c->next = NULL;

-            if (!vshCommandCheckOpts(ctl, c)) {
+            if (vshCommandCheckOpts(ctl, c, opts_required, opts_seen) < 0) {
                 VIR_FREE(c);
                 goto syntaxError;
             }
-- 
1.7.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]