These changes all look good to me, I really like the unit32_t-as-a-stack
idea.
---Peter
On Tue, 03 Mar 2015 21:41:11 -0800: Ben Pfaff <[email protected]> wrote:
On Mon, Feb 23, 2015 at 08:48:59AM -0800, Alex Wang wrote:
>> From: Peter Amidon <[email protected]>
>>
>> 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 <[email protected]>
> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
