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(a)redhat.com&gt; --- 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 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(a)redhat.com&gt; --- 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

Instead of freeing @partial and @buf explicitly, we can use g_auto*() to do that automatically. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

On a Tuesday in 2021, Michal Privoznik wrote: *its *its Jano