[libvirt] [PATCH 4/8] virsh: add vrshCommandParser abstraction

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:
>
> add vrshCommandParser and make vshCommandParse() accepts different

s/vrsh/vsh/; s/accepts/accept/

> parsers.
>
> the current code for parse command string is integrated as
> vshCommandStringParse().
>
> Signed-off-by: Lai Jiangshan<laijs at cn.fujitsu.com>
> ---
>   virsh.c |   91 +++++++++++++++++++++++++++++++---------------------------------
>   1 file changed, 45 insertions(+), 46 deletions(-)
> diff --git a/tools/virsh.c b/tools/virsh.c
> index f97ee42..27321a5 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -10158,23 +10158,29 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
>       return ret;
>   }
>
> +#define VSH_TK_ERROR       -1
> +#define VSH_TK_ARG         0
> +#define VSH_TK_SUBCMD_END  1
> +#define VSH_TK_END         2

Hmm, in addition to floating this earlier, you also lost _NONE and added 
_SUBCMD_END.  The enum I suggested in patch 3 is sounding more 
appealing, so I'll squash it in now.

> +
> +typedef struct __vshCommandParser {
> +    int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
> +    char *pos;
> +} vshCommandParser;
> +
> +static int vshCommandParse(vshControl *ctl, vshCommandParser *parser);

I'm a fan of avoiding forward static declarations if the file can be 
rearranged topologically.

> +
>   /* ---------------
>    * Command string parsing
>    * ---------------
>    */

This logically belongs before enums related to command parsing.

> -#define VSH_TK_ERROR    -1
> -#define VSH_TK_NONE    0
> -#define VSH_TK_DATA    1
> -#define VSH_TK_END    2
>
>   static int
> -vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
> +vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res)
>   {
>       bool double_quote = false;
>       int sz = 0;
> -    char *p = str, *q;
> -
> -    *end = NULL;
> +    char *p = parser->pos, *q;

Rebasing this on top of my edits to patch 1 is getting interesting.

>
>       while (p&&  *p&&  (*p == ' ' || *p == '\t'))

You no longer need to check if p is NULL,

> +static int vshCommandStringParse(vshControl *ctl, char *cmdstr)
> +{
> +    vshCommandParser parser;
> +
> +    if (cmdstr == NULL || *cmdstr == '\0')
> +        return FALSE;
> +
> +    parser.pos = cmdstr;

...since this guarantees it is non-NULL.

> +    for (;;) {

$ git grep 'for.*;;' | wc -l
14
$ git grep 'while.*true' | wc -l
6
$ git grep 'while.*1' | wc -l
70

Guess which one I'm changing this to, for consistency :)

ACK, with those nits fixed.  Here's what I squashed in:

diff --git i/tools/virsh.c w/tools/virsh.c
index 56985a4..d9f72f3 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10174,38 +10174,40 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
      return ret;
  }

-#define VSH_TK_ERROR       -1
-#define VSH_TK_ARG         0
-#define VSH_TK_SUBCMD_END  1
-#define VSH_TK_END         2
+/* ---------------
+ * Command string parsing
+ * ---------------
+ */
+
+typedef enum {
+    VSH_TK_ERROR, /* Failed to parse a token */
+    VSH_TK_ARG, /* Arbitrary argument, might be option or empty */
+    VSH_TK_SUBCMD_END, /* Separation between commands */
+    VSH_TK_END /* No more commands */
+} vshCommandToken;

  typedef struct __vshCommandParser {
-    int (*getNextArg)(vshControl *, struct __vshCommandParser *, char **);
+    vshCommandToken (*getNextArg)(vshControl *, struct 
__vshCommandParser *,
+                                  char **);
      char *pos;
  } vshCommandParser;

  static int vshCommandParse(vshControl *ctl, vshCommandParser *parser);

-/* ---------------
- * Command string parsing
- * ---------------
- */
-
-static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+static vshCommandToken ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
  vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char 
**res)
  {
      bool double_quote = false;
      int sz = 0;
      char *p = parser->pos;
-    char *q = vshStrdup(ctl, str);
+    char *q = vshStrdup(ctl, p);

-    *end = NULL;
      *res = q;

-    while (p && *p && (*p == ' ' || *p == '\t'))
+    while (*p && (*p == ' ' || *p == '\t'))
          p++;

-    if (p == NULL || *p == '\0')
+    if (*p == '\0')
          return VSH_TK_END;
      if (*p == ';') {
          parser->pos = ++p;             /* = \0 or begin of next command */
@@ -10261,15 +10262,15 @@ vshCommandParse(vshControl *ctl, 
vshCommandParser *parser)
          ctl->cmd = NULL;
      }

-    for (;;) {
+    while (1) {
          vshCmdOpt *last = NULL;
          const vshCmdDef *cmd = NULL;
-        int tk;
+        vshCommandToken tk;
          int data_ct = 0;

          first = NULL;

-        for (;;) {
+        while (1) {
              const vshCmdOptDef *opt = NULL;

              tkdata = NULL;

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org