[PATCH 0/9] Fix a slightly rework auto completion
These fixes were inspired by: https://gitlab.com/libvirt/libvirt/-/issues/116 Michal Prívozník (9): vshCommandStringGetArg: Drop @sz vsh: Don't break word on backslash vshReadlineParse: Bring some variables into !state block vshReadlineParse: Use g_auto*() vshReadlineParse: Rename @buf to @line vshReadlineParse: Escape list of candidates earlier vsh: Rework how option to complete is found vsh: Allow double quotes imbalance for auto completion in vshCommandStringGetArg() tools: Set IFS for bash completion script tools/bash-completion/vsh | 2 +- tools/virsh.c | 4 +- tools/virt-admin.c | 4 +- tools/vsh.c | 125 ++++++++++++++++++++++++-------------- tools/vsh.h | 5 +- 5 files changed, 88 insertions(+), 52 deletions(-) -- 2.26.2
This variable is unused since introduction of the function in v0.8.5~150. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index badd37c08e..54253afa72 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1608,7 +1608,6 @@ vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res, { bool single_quote = false; bool double_quote = false; - int sz = 0; char *p = parser->pos; char *q = g_strdup(p); @@ -1662,7 +1661,6 @@ vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res, } *q++ = *p++; - sz++; } if (double_quote) { if (report) -- 2.26.2
A backslash is the way we escape characters in virsh. For instance: virsh # start domain\ with\ long\ name For readline completion, we do not want to get four separate words ("domain", "with", "long", "name"). This means, that we can't sue virBufferEscapeShell() because it doesn't escape spaces the way we want. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 54253afa72..9856088126 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2773,7 +2773,7 @@ vshReadlineParse(const char *text, int state) if (ret && !rl_completion_quote_character) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - virBufferEscapeShell(&buf, ret); + virBufferEscape(&buf, '\\', " ", "%s", ret); VIR_FREE(ret); ret = virBufferContentAndReset(&buf); } @@ -2819,7 +2819,7 @@ vshReadlineInit(vshControl *ctl) int ret = -1; char *histsize_env = NULL; const char *histsize_str = NULL; - const char *break_characters = " \t\n\\`@$><=;|&{("; + const char *break_characters = " \t\n`@$><=;|&{("; const char *quote_characters = "\"'"; /* Opaque data for autocomplete callbacks. */ -- 2.26.2
On a Tuesday in 2021, Michal Privoznik wrote:
A backslash is the way we escape characters in virsh. For instance:
virsh # start domain\ with\ long\ name
For readline completion, we do not want to get four separate words ("domain", "with", "long", "name"). This means, that we can't sue virBufferEscapeShell() because it doesn't escape spaces
Let's leave the lawyers out of this. Jano
the way we want.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 54253afa72..9856088126 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2773,7 +2773,7 @@ vshReadlineParse(const char *text, int state) if (ret && !rl_completion_quote_character) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - virBufferEscapeShell(&buf, ret); + virBufferEscape(&buf, '\\', " ", "%s", ret); VIR_FREE(ret); ret = virBufferContentAndReset(&buf); } @@ -2819,7 +2819,7 @@ vshReadlineInit(vshControl *ctl) int ret = -1; char *histsize_env = NULL; const char *histsize_str = NULL; - const char *break_characters = " \t\n\\`@$><=;|&{("; + const char *break_characters = " \t\n`@$><=;|&{("; const char *quote_characters = "\"'";
/* Opaque data for autocomplete callbacks. */
On readline completion vshReadlineCompletion() is called which does nothing more than calling rl_completion_matches() with vshReadlineParse() as a callback. This means, that vshReadlineParse() is called repeatedly, each time returning next completion candidate, until it returns NULL which is interpreted as the end of the list of candidates. The function takes two parameters: @text which is a portion of input line around cursor when TAB was pressed, and @state. The @state is an integer that is zero on the very first call and non-zero on each subsequent call (in fact, readline does @state++ on each call). Anyway, the idea is that the callback gets the whole list of candidates on @state == 0 and returns one candidate at each call. And this is what vshReadlineParse() is doing but some variables (@parital, @cmd and @opt) are really used only in the @state == 0 case but declared for whole function. We can limit their scope by declaring them inside the @state == 0 body which also means that they don't have to be static anymore. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 9856088126..ba6299aae4 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2695,18 +2695,20 @@ vshCompleterFilter(char ***list, static char * vshReadlineParse(const char *text, int state) { - static vshCmd *partial; static char **list; static size_t list_index; - const vshCmdDef *cmd = NULL; - const vshCmdOptDef *opt = NULL; char *ret = NULL; + /* Readline calls this function until NULL is returned. On + * the very first call @state is zero which means we should + * initialize those static variables above. On subsequent + * calls @state is non zero. */ if (!state) { + vshCmd *partial = NULL; + const vshCmdDef *cmd = NULL; + const vshCmdOptDef *opt = NULL; char *buf = g_strdup(rl_line_buffer); - vshCommandFree(partial); - partial = NULL; g_strfreev(list); list = NULL; list_index = 0; @@ -2734,9 +2736,7 @@ vshReadlineParse(const char *text, int state) } opt = vshReadlineCommandFindOpt(partial, text); - } - if (!list) { if (!cmd) { list = vshReadlineCommandGenerator(text); } else { @@ -2759,10 +2759,12 @@ vshReadlineParse(const char *text, int state) (vshCompleterFilter(&completer_list, text) < 0 || virStringListMerge(&list, &completer_list) < 0)) { g_strfreev(completer_list); + vshCommandFree(partial); goto cleanup; } } } + vshCommandFree(partial); } if (list) { @@ -2780,15 +2782,12 @@ vshReadlineParse(const char *text, int state) cleanup: if (!ret) { - vshCommandFree(partial); - partial = NULL; g_strfreev(list); list = NULL; list_index = 0; } return ret; - } static char ** -- 2.26.2
On a Tuesday in 2021, Michal Privoznik wrote:
On readline completion vshReadlineCompletion() is called which does nothing more than calling rl_completion_matches() with vshReadlineParse() as a callback. This means, that vshReadlineParse() is called repeatedly, each time returning next completion candidate, until it returns NULL which is interpreted as the end of the list of candidates. The function takes two parameters: @text which is a portion of input line around cursor when TAB was pressed, and @state. The @state is an integer that is zero on the very first call and non-zero on each subsequent call (in fact, readline does @state++ on each call). Anyway, the idea is that the callback gets the whole list of candidates on @state == 0 and returns one candidate at each call. And this is what vshReadlineParse() is doing but some variables (@parital,
* partial Also, the commit message could use some newlines. Jano
@cmd and @opt) are really used only in the @state == 0 case but declared for whole function. We can limit their scope by declaring them inside the @state == 0 body which also means that they don't have to be static anymore.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
Instead of freeing @partial and @buf explicitly, we can use g_auto*() to do that automatically. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index ba6299aae4..9a7ca6776b 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -739,6 +739,8 @@ vshCommandFree(vshCmd *cmd) } } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(vshCmd, vshCommandFree); + /** * vshCommandOpt: * @cmd: parsed command line to search @@ -2704,10 +2706,10 @@ vshReadlineParse(const char *text, int state) * initialize those static variables above. On subsequent * calls @state is non zero. */ if (!state) { - vshCmd *partial = NULL; + g_autoptr(vshCmd) partial = NULL; const vshCmdDef *cmd = NULL; const vshCmdOptDef *opt = NULL; - char *buf = g_strdup(rl_line_buffer); + g_autofree char *buf = g_strdup(rl_line_buffer); g_strfreev(list); list = NULL; @@ -2717,8 +2719,6 @@ vshReadlineParse(const char *text, int state) vshCommandStringParse(NULL, buf, &partial); - VIR_FREE(buf); - if (partial) { cmd = partial->def; partial->skipChecks = true; @@ -2759,12 +2759,10 @@ vshReadlineParse(const char *text, int state) (vshCompleterFilter(&completer_list, text) < 0 || virStringListMerge(&list, &completer_list) < 0)) { g_strfreev(completer_list); - vshCommandFree(partial); goto cleanup; } } } - vshCommandFree(partial); } if (list) { -- 2.26.2
In next commit the block that does escaping of returned string will be brought into this block. But both contain variable @buf and use it in different contexts. Rename @buf from @state == 0 block to @line which reflects its purpose better. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 9a7ca6776b..abbd323e24 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2709,15 +2709,15 @@ vshReadlineParse(const char *text, int state) g_autoptr(vshCmd) partial = NULL; const vshCmdDef *cmd = NULL; const vshCmdOptDef *opt = NULL; - g_autofree char *buf = g_strdup(rl_line_buffer); + g_autofree char *line = g_strdup(rl_line_buffer); g_strfreev(list); list = NULL; list_index = 0; - *(buf + rl_point) = '\0'; + *(line + rl_point) = '\0'; - vshCommandStringParse(NULL, buf, &partial); + vshCommandStringParse(NULL, line, &partial); if (partial) { cmd = partial->def; -- 2.26.2
The way our completer callbacks work is that they return all possible candidates and then vshCompleterFilter() is called to prune the list of all candidates removing those which don't match user's input. This allows us to have simpler completer callbacks as their only job is to fetch all possible candidates. Anyway, if the completion candidate we're returning contains a space, it has to be escaped (shell like escaping), unless there is already a quote character (single quote or double quote). But ordering is critical. Completer callback returns string without any escaping, but the filter function sees the user input escaped. For instance, if user's input is "domain with space<TAB>" then the filtering function gets "domain\ with\ space" as user's input but completer returns "domain with space". Since these two strings don't match the filtering function removes this candidate from the list. What we need to do is to escape strings before calling the filtering function. This way, the filtering function will see two same strings. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index abbd323e24..27c201389d 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2751,10 +2751,26 @@ vshReadlineParse(const char *text, int state) partial, opt->completer_flags); + /* Escape completions, if needed (i.e. argument + * we completing wasn't started with a quote + * character). This also enables filtering done + * below to work properly. */ + if (completer_list && + !rl_completion_quote_character) { + size_t i; + + for (i = 0; completer_list[i]; i++) { + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferEscape(&buf, '\\', " ", "%s", completer_list[i]); + VIR_FREE(completer_list[i]); + completer_list[i] = virBufferContentAndReset(&buf); + } + } + /* For string list returned by completer we have to do * filtering based on @text because completer returns all * possible strings. */ - if (completer_list && (vshCompleterFilter(&completer_list, text) < 0 || virStringListMerge(&list, &completer_list) < 0)) { @@ -2770,14 +2786,6 @@ vshReadlineParse(const char *text, int state) list_index++; } - if (ret && - !rl_completion_quote_character) { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - virBufferEscape(&buf, '\\', " ", "%s", ret); - VIR_FREE(ret); - ret = virBufferContentAndReset(&buf); - } - cleanup: if (!ret) { g_strfreev(list); -- 2.26.2
On a Tuesday in 2021, Michal Privoznik wrote:
The way our completer callbacks work is that they return all possible candidates and then vshCompleterFilter() is called to prune the list of all candidates removing those which don't match user's input. This allows us to have simpler completer callbacks as their only job is to fetch all possible candidates.
Anyway, if the completion candidate we're returning contains a space, it has to be escaped (shell like escaping), unless there is already a quote character (single quote or double quote).
But ordering is critical. Completer callback returns string without any escaping, but the filter function sees the user input escaped. For instance, if user's input is "domain with space<TAB>" then the filtering function gets "domain\ with\ space" as user's input but completer returns "domain with space". Since these two strings don't match the filtering function removes this candidate from the list. What we need to do is to escape strings before calling the filtering function. This way, the filtering function will see two same strings.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index abbd323e24..27c201389d 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2751,10 +2751,26 @@ vshReadlineParse(const char *text, int state) partial, opt->completer_flags);
+ /* Escape completions, if needed (i.e. argument + * we completing wasn't started with a quote
we are completing Jano
+ * character). This also enables filtering done + * below to work properly. */ + if (completer_list && + !rl_completion_quote_character) { + size_t i; + + for (i = 0; completer_list[i]; i++) { + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferEscape(&buf, '\\', " ", "%s", completer_list[i]); + VIR_FREE(completer_list[i]); + completer_list[i] = virBufferContentAndReset(&buf); + } + } + /* For string list returned by completer we have to do * filtering based on @text because completer returns all * possible strings. */ - if (completer_list && (vshCompleterFilter(&completer_list, text) < 0 || virStringListMerge(&list, &completer_list) < 0)) {
The way that auto completion works currently is that user's input is parsed, and then we try to find the first --option (in the parsed structure) that has the same value as user's input around where <TAB> was pressed. For instance, for the following input: virsh # command --arg1 hello --arg2 world<TAB> we will see "world" as text that user is trying to autocomplete (this is affected by rl_basic_word_break_characters which readline uses internally to break user's input into individual words) and find that it is --arg2 that user is trying to autocomplete. So far so good, for this naive approach. But consider the following example: virsh # command --arg1 world --arg2 world<TAB> Here, both arguments have the same value and because we see "world" as text that user is trying to autocomplete we would think that it is --arg1 that user wants to autocomplete. This is obviously wrong. Fortunately, readline stores the current position of cursor (into rl_point) and we can use that when parsing user's input: whenever we reach a position that matches the cursor then we know that that is the place where <TAB> was pressed and hence that is the --option that user wants to autocomplete. Readline stores the cursor position as offset (numbered from 1) from the beginning of user's input. We store this input into @parser->pos initially, but then advance it as we tokenize it. Therefore, what we need is to store the original position too. Thanks to Martin who helped me with this. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh.c | 4 ++-- tools/virt-admin.c | 4 ++-- tools/vsh.c | 58 ++++++++++++++++++++++++++++++++-------------- tools/vsh.h | 5 +++- 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index a6bfbbbb87..732655a894 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -777,7 +777,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) ctl->imode = false; if (argc - optind == 1) { vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]); - return vshCommandStringParse(ctl, argv[optind], NULL); + return vshCommandStringParse(ctl, argv[optind], NULL, 0); } else { return vshCommandArgvParse(ctl, argc - optind, argv + optind); } @@ -915,7 +915,7 @@ main(int argc, char **argv) if (*ctl->cmdstr) { vshReadlineHistoryAdd(ctl->cmdstr); - if (vshCommandStringParse(ctl, ctl->cmdstr, NULL)) + if (vshCommandStringParse(ctl, ctl->cmdstr, NULL, 0)) vshCommandRun(ctl, ctl->cmd); } VIR_FREE(ctl->cmdstr); diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 72084a78e9..1108a07896 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1364,7 +1364,7 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv) ctl->imode = false; if (argc - optind == 1) { vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]); - return vshCommandStringParse(ctl, argv[optind], NULL); + return vshCommandStringParse(ctl, argv[optind], NULL, 0); } else { return vshCommandArgvParse(ctl, argc - optind, argv + optind); } @@ -1594,7 +1594,7 @@ main(int argc, char **argv) if (*ctl->cmdstr) { vshReadlineHistoryAdd(ctl->cmdstr); - if (vshCommandStringParse(ctl, ctl->cmdstr, NULL)) + if (vshCommandStringParse(ctl, ctl->cmdstr, NULL, 0)) vshCommandRun(ctl, ctl->cmd); } VIR_FREE(ctl->cmdstr); diff --git a/tools/vsh.c b/tools/vsh.c index 27c201389d..e5c6cebebb 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1318,6 +1318,8 @@ struct _vshCommandParser { char **, bool); /* vshCommandStringGetArg() */ char *pos; + const char *originalLine; + size_t point; /* vshCommandArgvGetArg() */ char **arg_pos; char **arg_end; @@ -1426,6 +1428,10 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial) arg->data = tkdata; tkdata = NULL; arg->next = NULL; + + if (parser->pos - parser->originalLine == parser->point - 1) + arg->completeThis = true; + if (!first) first = arg; if (last) @@ -1477,6 +1483,9 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial) arg->next = NULL; tkdata = NULL; + if (parser->pos - parser->originalLine == parser->point) + arg->completeThis = true; + if (!first) first = arg; if (last) @@ -1588,7 +1597,7 @@ vshCommandArgvGetArg(vshControl *ctl G_GNUC_UNUSED, bool vshCommandArgvParse(vshControl *ctl, int nargs, char **argv) { - vshCommandParser parser; + vshCommandParser parser = { 0 }; if (nargs <= 0) return false; @@ -1675,15 +1684,38 @@ vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res, return VSH_TK_ARG; } + +/** + * vshCommandStringParse: + * @ctl virsh control structure + * @cmdstr: string to parse + * @partial: store partially parsed command here + * @point: position of cursor (rl_point) + * + * Parse given string @cmdstr as a command and store it under + * @ctl->cmd. For readline completion, if @partial is not NULL on + * the input then errors in parsing are ignored (because user is + * still in progress of writing the command string) and partially + * parsed command is stored at *@partial (caller has to free it + * afterwards). Among with @partial, caller must set @point which + * is the position of cursor in @cmdstr (offset, numbered from 1). + * Parser will then set @completeThis attribute to true for the + * vshCmdOpt that appeared under the cursor. + */ bool -vshCommandStringParse(vshControl *ctl, char *cmdstr, vshCmd **partial) +vshCommandStringParse(vshControl *ctl, + char *cmdstr, + vshCmd **partial, + size_t point) { - vshCommandParser parser; + vshCommandParser parser = { 0 }; if (cmdstr == NULL || *cmdstr == '\0') return false; parser.pos = cmdstr; + parser.originalLine = cmdstr; + parser.point = point; parser.getNextArg = vshCommandStringGetArg; return vshCommandParse(ctl, &parser, partial); } @@ -2634,28 +2666,20 @@ vshReadlineOptionsGenerator(const char *text, static const vshCmdOptDef * -vshReadlineCommandFindOpt(const vshCmd *partial, - const char *text) +vshReadlineCommandFindOpt(const vshCmd *partial) { const vshCmd *tmp = partial; - while (tmp && tmp->next) { - if (tmp->def == tmp->next->def && - !tmp->next->opts) - break; - tmp = tmp->next; - } - - if (tmp && tmp->opts) { + while (tmp) { const vshCmdOpt *opt = tmp->opts; while (opt) { - if (STREQ_NULLABLE(opt->data, text) || - STREQ_NULLABLE(opt->data, " ")) + if (opt->completeThis) return opt->def; opt = opt->next; } + tmp = tmp->next; } return NULL; @@ -2717,7 +2741,7 @@ vshReadlineParse(const char *text, int state) *(line + rl_point) = '\0'; - vshCommandStringParse(NULL, line, &partial); + vshCommandStringParse(NULL, line, &partial, rl_point); if (partial) { cmd = partial->def; @@ -2735,7 +2759,7 @@ vshReadlineParse(const char *text, int state) cmd = NULL; } - opt = vshReadlineCommandFindOpt(partial, text); + opt = vshReadlineCommandFindOpt(partial); if (!cmd) { list = vshReadlineCommandGenerator(text); diff --git a/tools/vsh.h b/tools/vsh.h index 0c5584891d..39f70913fe 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -152,6 +152,8 @@ struct _vshCmdOptDef { struct _vshCmdOpt { const vshCmdOptDef *def; /* non-NULL pointer to option definition */ char *data; /* allocated data, or NULL for bool option */ + bool completeThis; /* true if this is the option user's wishing to + autocomplete */ vshCmdOpt *next; }; @@ -292,7 +294,8 @@ int vshBlockJobOptionBandwidth(vshControl *ctl, unsigned long *bandwidth); bool vshCommandOptBool(const vshCmd *cmd, const char *name); bool vshCommandRun(vshControl *ctl, const vshCmd *cmd); -bool vshCommandStringParse(vshControl *ctl, char *cmdstr, vshCmd **partial); +bool vshCommandStringParse(vshControl *ctl, char *cmdstr, + vshCmd **partial, size_t point); const vshCmdOpt *vshCommandOptArgv(vshControl *ctl, const vshCmd *cmd, const vshCmdOpt *opt); -- 2.26.2
If user is trying to auto complete a value that contains a space, they have two options: use backslash to escape space or use qotes, like this: virsh # start --domain "domain with space<TAB> However, in this case our tokenizer sees imbalance in (double) quotes: there is a starting one that's missing it's companion. Well, that's obvious - user is still in process of writing the command. What we need to do in this case is to ignore the imbalance and return success (from the tokenizer) - readline will handle closing the quote properly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index e5c6cebebb..53a84b9d95 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1418,7 +1418,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial) if (optstr) tkdata = optstr; else - tk = parser->getNextArg(ctl, parser, &tkdata, true); + tk = parser->getNextArg(ctl, parser, &tkdata, partial == NULL); if (tk == VSH_TK_ERROR) goto syntaxError; if (tk != VSH_TK_ARG) { @@ -1673,10 +1673,16 @@ vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res, *q++ = *p++; } + if (double_quote) { - if (report) + /* We have seen a double quote, but not it's companion + * ending. It's valid though, in case when we're called + * from completer (report = false), but it's not valid + * when parsing real command (report= true). */ + if (report) { vshError(ctl, "%s", _("missing \"")); - return VSH_TK_ERROR; + return VSH_TK_ERROR; + } } *q = '\0'; -- 2.26.2
On a Tuesday in 2021, Michal Privoznik wrote:
If user is trying to auto complete a value that contains a space, they have two options: use backslash to escape space or use qotes, like this:
virsh # start --domain "domain with space<TAB>
However, in this case our tokenizer sees imbalance in (double) quotes: there is a starting one that's missing it's companion.
*its
Well, that's obvious - user is still in process of writing the command. What we need to do in this case is to ignore the imbalance and return success (from the tokenizer) - readline will handle closing the quote properly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index e5c6cebebb..53a84b9d95 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1418,7 +1418,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial) if (optstr) tkdata = optstr; else - tk = parser->getNextArg(ctl, parser, &tkdata, true); + tk = parser->getNextArg(ctl, parser, &tkdata, partial == NULL); if (tk == VSH_TK_ERROR) goto syntaxError; if (tk != VSH_TK_ARG) { @@ -1673,10 +1673,16 @@ vshCommandStringGetArg(vshControl *ctl, vshCommandParser *parser, char **res,
*q++ = *p++; } + if (double_quote) { - if (report) + /* We have seen a double quote, but not it's companion
*its Jano
+ * ending. It's valid though, in case when we're called + * from completer (report = false), but it's not valid + * when parsing real command (report= true). */ + if (report) { vshError(ctl, "%s", _("missing \"")); - return VSH_TK_ERROR; + return VSH_TK_ERROR; + } }
*q = '\0'; -- 2.26.2
The way our bash completion string is that is gets user's input and lets virsh completion code do all the work by calling 'virsh complete -- $INPUT". The 'complete' command is a "secret", unlisted command that exists solely for this purpose. After it has done it's part, it prints candidates onto stdout, each candidate on its own line, e.g. like this: # virsh complete -- "net-u" net-undefine net-update net-uuid These strings are then stored into a bash array $A like this: A=($($1 ${CMDLINE} complete -- "${INPUT[@]}" 2>/dev/null)) This array is then thrown back at bash completion to produce desired output. So far so good. Except, when there is an option with space. For instance: # virsh complete -- start --domain "" uefi\ duplicate uefi Bash interprets that as another array item because by default, Internal Field Separator (IFS) = set of characters that bash uses to split words at, is: space, TAB, newline. We don't want space nor TAB. Therefore, we have to set $IFS when storing 'virsh complete' output into the array. Thanks to Peter who suggested it. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/116 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/bash-completion/vsh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/bash-completion/vsh b/tools/bash-completion/vsh index fb38e8616f..bbb25fc3eb 100644 --- a/tools/bash-completion/vsh +++ b/tools/bash-completion/vsh @@ -56,7 +56,7 @@ _vsh_complete() # the name of the command whose arguments are being # completed. # Therefore, we might just run $1. - A=($($1 ${CMDLINE} complete -- "${INPUT[@]}" 2>/dev/null)) + IFS=$'\n' A=($($1 ${CMDLINE} complete -- "${INPUT[@]}" 2>/dev/null)) COMPREPLY=($(compgen -W "${A[*]%--}" -- ${cur})) __ltrim_colon_completions "$cur" -- 2.26.2
On a Tuesday in 2021, Michal Privoznik wrote:
These fixes were inspired by:
https://gitlab.com/libvirt/libvirt/-/issues/116
Michal Prívozník (9): vshCommandStringGetArg: Drop @sz vsh: Don't break word on backslash vshReadlineParse: Bring some variables into !state block vshReadlineParse: Use g_auto*() vshReadlineParse: Rename @buf to @line vshReadlineParse: Escape list of candidates earlier vsh: Rework how option to complete is found vsh: Allow double quotes imbalance for auto completion in vshCommandStringGetArg() tools: Set IFS for bash completion script
tools/bash-completion/vsh | 2 +- tools/virsh.c | 4 +- tools/virt-admin.c | 4 +- tools/vsh.c | 125 ++++++++++++++++++++++++-------------- tools/vsh.h | 5 +- 5 files changed, 88 insertions(+), 52 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik