[ovs-dev] [RFC 2/3] ovs-vsctl-bashcomp: Add bash command-line completion for ovs-vsctl.

Ben Pfaff blp at nicira.com
Wed Mar 4 05:41:11 UTC 2015


On Mon, Feb 23, 2015 at 08:48:59AM -0800, Alex Wang wrote:
> From: Peter Amidon <peter at picnicpark.org>
> 
> This patch adds bash command-line completion script for ovs-vsctl.
> Therein, codes are added to ovs-vsctl to allow it to print the
> options and command arguments.  The ovs-vsctl-bashcomp.bash will
> parse the vsctl command and complete on the user input.
> 
> The completion script can do the following::
> 
> - display available completion and complete on user input for
>   global/local options, command, and argument.
> 
> - query database and expand keywords like 'table/record/column/key'
>   to available completions.
> 
> - deal with argument relations like 'one and more', 'zero or one'.
> 
> - complete multiple ovs-vsctl commands cascaded via '--'.
> 
> To use the script, either copy it inside /etc/bash_completion.d/
> or manually run it via . ovs-vsctl-bashcomp.bash.
> 
> Signed-off-by: Alex Wang <alexw at nicira.com>

Since I'm not a bash expert, I think I'll concentrate on the C code in
ovs-vsctl.c.  Alex, have you reviewed the bash code?  As long as you
have, I'm happy with it.

valgrind reports a buffer overrun when the length of command->argument
is 0.

It appears that print_command_arguments() uses a linked list to maintain
a stack of bools that will never get very deep.  I think a single-word
bitmap would work just as well.

Here, you can omit the multiplication by sizeof(char), because the C
standard guarantees that sizeof(char) is 1:
    char *simple_args = xzalloc(2 * length * sizeof(char));

'in_repeated' should be declared bool and assigned true and false (not 0
and 1).

Please write a space around binary operators, e.g. arguments[length-i-1]
becomes arguments[length - i - 1].

It took me a minute to figure out what was going on with the output
buffer, that it's actually being written backward byte-by-byte.  That's
brilliant, but I found the actual bookkeeping method difficult to
follow.  This is a case where I find it a lot easier to understand when
there's a pointer to the current position that we decrement as we go.

Pretty much the same for the input buffer, although that's not quite as
hard to understand at first glance.
 
I think there's a missing "free" of the output buffer too.

There are double blank lines between functions.  We don't generally do
that.

strlen(item) > 0 is more efficiently written item[0] != '\0'.

I usually try to write "break;" after every case in a switch statement,
even the last one, because it makes it harder to forget to add one if
you add another case later.

s/surronds/surrounds/ in the comment here:
     * We use 'whole_word_is_optional' value to decide whether or not a ! or
     * + should be added on encountering a space: if the optional surronds
     * the whole word then it shouldn't be, but if it is only a part of the

I got interested in this so I actually implemented these changes.
Here's an incremental diff.  I verified that it produces the same output
as the original version.

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 695ecd4..efd138c 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -768,94 +768,75 @@ static void
 print_command_arguments(const struct vsctl_command_syntax *command)
 {
     /*
-     * The argument string is parsed in reverse.  We use a stack 'oew_stack'
-     * to keep track of nested optionals.  Whenever a ']' is encountered, we
-     * push an element to 'oew_stack'.  The 'optional_ends_word' is set if
-     * the ']' is not nested.  Subsequently, we pop an entry everytime '[' is
-     * met.
+     * The argument string is parsed in reverse.  We use a stack 'oew_stack' to
+     * keep track of nested optionals.  Whenever a ']' is encountered, we push
+     * a bit to 'oew_stack'.  The bit is set to 1 if the ']' is not nested.
+     * Subsequently, we pop an entry everytime '[' is met.
      *
-     * We use 'whole_word_is_optional' value to decide whether or not a ! or
-     * + should be added on encountering a space: if the optional surronds
-     * the whole word then it shouldn't be, but if it is only a part of the
-     * word (i.e. [key=]value), it should be.
+     * We use 'whole_word_is_optional' value to decide whether or not a ! or +
+     * should be added on encountering a space: if the optional surrounds the
+     * whole word then it shouldn't be, but if it is only a part of the word
+     * (i.e. [key=]value), it should be.
      */
-    struct oew_stack_element {
-        bool optional_ends_word;
-        struct ovs_list node;
-    };
+    uint32_t oew_stack = 0;
 
     const char *arguments = command->arguments;
-    struct ovs_list oew_stack;
     int length = strlen(arguments);
-    char *simple_args = xzalloc(2 * length * sizeof(char));
-    /* One char has already been written: \0 */
-    int chars_written = 1;
-    int in_repeated = 0;
-    bool whole_word_is_optional = false;
-    int i;
+    if (!length) {
+        return;
+    }
 
-    list_init(&oew_stack);
+    /* Output buffer, written backward from end. */
+    char *output = xmalloc(2 * length);
+    char *outp = output + 2 * length;
+    *--outp = '\0';
 
-    for (i = 1; i <= length; i++) {
-        int simple_index = 2 * length - chars_written - 1;
-        char current = arguments[length-i];
-        struct oew_stack_element *elem;
-        int oew;
+    bool in_repeated = false;
+    bool whole_word_is_optional = false;
 
-        switch(current) {
+    for (const char *inp = arguments + length; inp > arguments; ) {
+        switch (*--inp) {
         case ']':
-            elem = xmalloc(sizeof *elem);
-            if (i == 1
-                || arguments[length-i+1] == ' '
-                || arguments[length-i+1] == '.') {
-                elem->optional_ends_word = true;
-            } else {
-                elem->optional_ends_word = false;
+            oew_stack <<= 1;
+            if (inp[1] == '\0' || inp[1] == ' ' || inp[1] == '.') {
+                oew_stack |= 1;
             }
-            list_push_back(&oew_stack, &elem->node);
             break;
         case '[':
-            elem = CONTAINER_OF(list_pop_back(&oew_stack),
-                                struct oew_stack_element,
-                                node);
-            oew = elem->optional_ends_word;
-            free(elem);
             /* Checks if the whole word is optional, and sets the
              * 'whole_word_is_optional' accordingly. */
-            if ((i == length || arguments[length-i-1] == ' ')
-                && oew) {
-                simple_args[simple_index] = in_repeated ? '*' : '?';
-                whole_word_is_optional = oew;
+            if ((inp == arguments || inp[-1] == ' ') && oew_stack & 1) {
+                *--outp = in_repeated ? '*' : '?';
+                whole_word_is_optional = true;
             } else {
-                simple_args[simple_index] = '?';
-                whole_word_is_optional = 0;
+                *--outp = '?';
+                whole_word_is_optional = false;
             }
-            chars_written++;
+            oew_stack >>= 1;
             break;
         case ' ':
             if (!whole_word_is_optional) {
-                simple_args[simple_index] = in_repeated ? '+' : '!';
-                chars_written++;
+                *--outp = in_repeated ? '+' : '!';
             }
-            simple_args[2 * length - ++chars_written] = ' ';
-            in_repeated = 0;
+            *--outp = ' ';
+            in_repeated = false;
             whole_word_is_optional = false;
             break;
         case '.':
-            in_repeated = 1;
+            in_repeated = true;
             break;
         default:
-            simple_args[simple_index] = current;
-            chars_written++;
+            *--outp = *inp;
+            break;
         }
     }
-    if (arguments[0] != '[' && chars_written > 1) {
-        simple_args[2 * length - ++chars_written] = in_repeated ? '+' : '!';
+    if (arguments[0] != '[' && outp != output + 2 * length - 1) {
+        *--outp = in_repeated ? '+' : '!';
     }
-    printf("%s", simple_args + 2 * length - chars_written);
+    printf("%s", outp);
+    free(output);
 }
 
-
 static void
 print_vsctl_commands(void)
 {
@@ -866,10 +847,9 @@ print_vsctl_commands(void)
         char *options_begin = options;
         char *item;
 
-        for (item = strsep(&options, ",");
-             item != NULL;
+        for (item = strsep(&options, ","); item != NULL;
              item = strsep(&options, ",")) {
-            if (strlen(item) > 0) {
+            if (item[0] != '\0') {
                 printf("[%s] ", item);
             }
         }
@@ -883,7 +863,6 @@ print_vsctl_commands(void)
     exit(EXIT_SUCCESS);
 }
 
-
 static void
 print_vsctl_options(const struct option *options)
 {
@@ -899,7 +878,6 @@ print_vsctl_options(const struct option *options)
     exit(EXIT_SUCCESS);
 }
 
-
 static char *
 default_db(void)
 {



More information about the dev mailing list