Cool, I'll adopt the change and resend patches,
On Wed, Mar 4, 2015 at 6:33 PM, Peter Amidon <pe...@picnicpark.org> wrote:
> 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 <b...@nicira.com> wrote:
>
>
> 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
