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

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



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 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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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