[ovs-dev] [RFC 2/3] ovs-vsctl-bashcomp: Add bash command-line completion for ovs-vsctl.
Alex Wang
alexw at nicira.com
Wed Mar 4 18:34:25 UTC 2015
Thx for the close review, Ben!
Really likes the use of uint32_t ask stack.
I reviewed the bash scripts. And composed the tests based on my
understanding of the scripts.
Your changes look good to me. I'd wait for Peter to check your changes.
And see what he thinks.
Thanks,
Alex Wang,
On Tue, Mar 3, 2015 at 9:41 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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