On Mon, Feb 23, 2015 at 08:48:59AM -0800, Alex Wang wrote:
> From: Peter Amidon <pe...@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 <al...@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)
{
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
