[PATCH 00/12] vsh: Improve completer code
There is no functional change to completion. No user visible change, nor to a developer who writes a completer callback. Some code deduplication and code cleanup and adapting code to 2021. Michal Prívozník (12): lib: Substitute some STREQLEN with STRPREFIX vsh: Don't put VSH_OT_ALIAS onto list of completions vsh: Use g_auto(GStrv) to free string list returned by completer callback vsh: Accept NULL @list in vshCompleterFilter() vsh: Prefer g_strdup_printf() over g_snprintf() in vshReadlineOptionsGenerator() vsh: Use g_auto() for string lists returned in readline command/options generators vsh: Rewrite opt->type check in vshReadlineParse() vsh: Deduplicate filtering in vshReadlineOptionsGenerator() vsh: Deduplicate filtering in vshReadlineCommandGenerator() vsh: Simplify condition for calling completer callback vsh: Rework vshReadlineCommandGenerator() vsh: Drop unused @text arg from readline generators src/libxl/xen_xl.c | 2 +- src/util/vircgroupv2.c | 10 +-- tools/vsh.c | 152 ++++++++++++++++------------------------- 3 files changed, 63 insertions(+), 101 deletions(-) -- 2.26.2
There are few cases where STREQLEN() is called like this: STREQLEN(var, string, strlen(string)) which is the same as STRPREFIX(var, string). Use STRPREFIX() because it is more obvious what the check is doing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 2 +- src/util/vircgroupv2.c | 10 +++++----- tools/vsh.c | 3 +-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 941832ce4e..69b139354e 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -950,7 +950,7 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def) else usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; } else { - if (STREQLEN(type, "qusb", 4)) { + if (STRPREFIX(type, "qusb")) { if (usbctrl_version == 1) usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1; else diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 4a239f067a..49acd27714 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -938,7 +938,7 @@ virCgroupV2GetBlkioDeviceReadIops(virCgroupPtr group, } tmp += strlen(name); - if (STREQLEN(tmp, "max", 3)) { + if (STRPREFIX(tmp, "max")) { *riops = 0; } else if (virStrToLong_ui(tmp, &tmp, 10, riops) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1007,7 +1007,7 @@ virCgroupV2GetBlkioDeviceWriteIops(virCgroupPtr group, } tmp += strlen(name); - if (STREQLEN(tmp, "max", 3)) { + if (STRPREFIX(tmp, "max")) { *wiops = 0; } else if (virStrToLong_ui(tmp, &tmp, 10, wiops) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1076,7 +1076,7 @@ virCgroupV2GetBlkioDeviceReadBps(virCgroupPtr group, } tmp += strlen(name); - if (STREQLEN(tmp, "max", 3)) { + if (STRPREFIX(tmp, "max")) { *rbps = 0; } else if (virStrToLong_ull(tmp, &tmp, 10, rbps) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1145,7 +1145,7 @@ virCgroupV2GetBlkioDeviceWriteBps(virCgroupPtr group, } tmp += strlen(name); - if (STREQLEN(tmp, "max", 3)) { + if (STRPREFIX(tmp, "max")) { *wbps = 0; } else if (virStrToLong_ull(tmp, &tmp, 10, wbps) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1578,7 +1578,7 @@ virCgroupV2GetCpuCfsQuota(virCgroupPtr group, return -1; } - if (STREQLEN(str, "max", 3)) { + if (STRPREFIX(str, "max")) { *cfs_quota = VIR_CGROUP_CPU_QUOTA_MAX; return 0; } diff --git a/tools/vsh.c b/tools/vsh.c index 0e2d4955b4..a242619b8a 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2568,7 +2568,6 @@ static char ** vshReadlineCommandGenerator(const char *text) { size_t grp_list_index = 0, cmd_list_index = 0; - size_t len = strlen(text); const char *name; const vshCmdGrp *grp; const vshCmdDef *cmds; @@ -2588,7 +2587,7 @@ vshReadlineCommandGenerator(const char *text) if (cmds[cmd_list_index++].flags & VSH_CMD_FLAG_ALIAS) continue; - if (STREQLEN(name, text, len)) { + if (STRPREFIX(name, text)) { if (VIR_REALLOC_N(ret, ret_size + 2) < 0) { g_strfreev(ret); return NULL; -- 2.26.2
We've invented VSH_OT_ALIAS type for --option so that we can rewrite some --options (e.g. fix spelling). For instance blkdeviotune command uses this feature heavily: --options-with-dash are preferred over old --options_with_underscore. Both versions are supported but only the new ones (not aliased) are documented and reported in --help. Except for options completer, which happily put also aliased versions in front of user's eyes. Note, there is a second (gross) way we use aliases: to rewrite options from --oldoption to --newoption=value (for instance --shareable option of attach-disk is an alias of --mode=shareable). And just like with the previous group - don't generate them into the list of possible options. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index a242619b8a..cc777bee12 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2614,7 +2614,6 @@ vshReadlineOptionsGenerator(const char *text, { size_t list_index = 0; size_t len = strlen(text); - const char *name; size_t ret_size = 0; char **ret = NULL; @@ -2624,12 +2623,15 @@ vshReadlineOptionsGenerator(const char *text, if (!cmd->opts) return NULL; - while ((name = cmd->opts[list_index].name)) { + for (list_index = 0; cmd->opts[list_index].name; list_index++) { + const char *name = cmd->opts[list_index].name; bool exists = false; vshCmdOpt *opt = last->opts; size_t name_len; - list_index++; + /* Skip aliases, we do not report them in help output either. */ + if (cmd->opts[list_index].type == VSH_OT_ALIAS) + continue; if (len > 2) { /* provide auto-complete only when the text starts with -- */ -- 2.26.2
This saves us explicit call of g_strfreev() in error path. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index cc777bee12..3cb657f582 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2778,9 +2778,9 @@ vshReadlineParse(const char *text, int state) list = vshReadlineOptionsGenerator(text, cmd, partial); if (opt && opt->completer) { - char **completer_list = opt->completer(autoCompleteOpaque, - partial, - opt->completer_flags); + g_auto(GStrv) completer_list = opt->completer(autoCompleteOpaque, + partial, + opt->completer_flags); /* Escape completions, if needed (i.e. argument * we are completing wasn't started with a quote @@ -2805,7 +2805,6 @@ vshReadlineParse(const char *text, int state) if (completer_list && (vshCompleterFilter(&completer_list, text) < 0 || virStringListMerge(&list, &completer_list) < 0)) { - g_strfreev(completer_list); goto cleanup; } } -- 2.26.2
The aim of vshCompleterFilter() is to take a string list and a prefix and remove all strings from the list that don't have the desired prefix. The function is used to filter out those strings returned by a completer callback that don't correspond with user's (partial) input. For instance, domain name completer virshDomainNameCompleter() returns all domain names and then vshCompleterFilter() refines the list so that only domains with correct prefix of their name are offered to user. This was a design choice - it allows us to have shorter completers as they do not have to copy the list filtering over and over. Having said all of that, it may happen that a completer does not return anything (e.g. there is no domain in requested state, virsh is not connected and thus completer exited early, etc.). In that case, the string list is NULL and vshCompleterFilter() can simply return early. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 3cb657f582..13bf525acc 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2703,7 +2703,7 @@ vshCompleterFilter(char ***list, size_t i; if (!list || !*list) - return -1; + return 0; list_len = virStringListLength((const char **) *list); newList = g_new0(char *, list_len + 1); @@ -2802,9 +2802,8 @@ vshReadlineParse(const char *text, int state) /* 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)) { + if (vshCompleterFilter(&completer_list, text) < 0 || + virStringListMerge(&list, &completer_list) < 0) { goto cleanup; } } -- 2.26.2
The vshReadlineOptionsGenerator() function returns a string list of all --options for given command. But the way that individual items on the list are allocated can be written better. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 13bf525acc..d7ab7c61db 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2627,7 +2627,6 @@ vshReadlineOptionsGenerator(const char *text, const char *name = cmd->opts[list_index].name; bool exists = false; vshCmdOpt *opt = last->opts; - size_t name_len; /* Skip aliases, we do not report them in help output either. */ if (cmd->opts[list_index].type == VSH_OT_ALIAS) @@ -2660,9 +2659,7 @@ vshReadlineOptionsGenerator(const char *text, return NULL; } - name_len = strlen(name); - ret[ret_size] = g_new0(char, name_len + 3); - g_snprintf(ret[ret_size], name_len + 3, "--%s", name); + ret[ret_size] = g_strdup_printf("--%s", name); ret_size++; /* Terminate the string list properly. */ ret[ret_size] = NULL; -- 2.26.2
There are two functions that are used to generate completion lists: vshReadlineCommandGenerator() for command names and vshReadlineOptionsGenerator() for --options for given command. Both return a string list, but may also fail while constructing it. For that case, they call g_strfreev() explicitly, which is needless since we have g_auto(GStrv). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index d7ab7c61db..70b7e3e285 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2572,7 +2572,7 @@ vshReadlineCommandGenerator(const char *text) const vshCmdGrp *grp; const vshCmdDef *cmds; size_t ret_size = 0; - char **ret = NULL; + g_auto(GStrv) ret = NULL; grp = cmdGroups; @@ -2588,10 +2588,9 @@ vshReadlineCommandGenerator(const char *text) continue; if (STRPREFIX(name, text)) { - if (VIR_REALLOC_N(ret, ret_size + 2) < 0) { - g_strfreev(ret); + if (VIR_REALLOC_N(ret, ret_size + 2) < 0) return NULL; - } + ret[ret_size] = g_strdup(name); ret_size++; /* Terminate the string list properly. */ @@ -2604,7 +2603,7 @@ vshReadlineCommandGenerator(const char *text) } } - return ret; + return g_steal_pointer(&ret); } static char ** @@ -2615,7 +2614,7 @@ vshReadlineOptionsGenerator(const char *text, size_t list_index = 0; size_t len = strlen(text); size_t ret_size = 0; - char **ret = NULL; + g_auto(GStrv) ret = NULL; if (!cmd) return NULL; @@ -2654,10 +2653,8 @@ vshReadlineOptionsGenerator(const char *text, if (exists) continue; - if (VIR_REALLOC_N(ret, ret_size + 2) < 0) { - g_strfreev(ret); + if (VIR_REALLOC_N(ret, ret_size + 2) < 0) return NULL; - } ret[ret_size] = g_strdup_printf("--%s", name); ret_size++; @@ -2665,7 +2662,7 @@ vshReadlineOptionsGenerator(const char *text, ret[ret_size] = NULL; } - return ret; + return g_steal_pointer(&ret); } -- 2.26.2
The vshReadlineParse() function is called whenever user hits <TAB><TAB>. If there is no command (or a partially written one), then a list of possible commands is printed to the user. But, if there is a command then its --options are generated. But obviously, we can not generate --options if there already is an --option that's expecting a value. For instance, consider: virsh # start --domain <TAB><TAB> In this case we want to call completer for --domain option, but that's a different story. Anyway, the way that we currently check whether --options list should be generated is checking the type of the last --option. If it isn't DATA, STRING, INT, or ARGV (all these expect a value), then we can generate --option list. Well, writing the condition this way is needlessly verbose and also prone to errors (see d9a320bf97 for example). We know that boolean type does not require a value. This leaves us with the only type that was not mentioned yet - VSH_OT_ALIAS. This is a special type for backwards compatibility and it refers to another --option which can be just any type. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 70b7e3e285..f83b2a95a9 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2765,10 +2765,7 @@ vshReadlineParse(const char *text, int state) if (!cmd) { list = vshReadlineCommandGenerator(text); } else { - if (!opt || (opt->type != VSH_OT_DATA && - opt->type != VSH_OT_STRING && - opt->type != VSH_OT_INT && - opt->type != VSH_OT_ARGV)) + if (!opt || opt->type == VSH_OT_BOOL) list = vshReadlineOptionsGenerator(text, cmd, partial); if (opt && opt->completer) { -- 2.26.2
This is what we do for completer callbacks: we let them generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix). The same algorithm is implemented in vshReadlineOptionsGenerator() even though a bit differently. There is no need to have the same code twice. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index f83b2a95a9..5f082f1a35 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2607,12 +2607,11 @@ vshReadlineCommandGenerator(const char *text) } static char ** -vshReadlineOptionsGenerator(const char *text, +vshReadlineOptionsGenerator(const char *text G_GNUC_UNUSED, const vshCmdDef *cmd, vshCmd *last) { size_t list_index = 0; - size_t len = strlen(text); size_t ret_size = 0; g_auto(GStrv) ret = NULL; @@ -2631,16 +2630,6 @@ vshReadlineOptionsGenerator(const char *text, if (cmd->opts[list_index].type == VSH_OT_ALIAS) continue; - if (len > 2) { - /* provide auto-complete only when the text starts with -- */ - if (STRNEQLEN(text, "--", 2)) - return NULL; - if (STRNEQLEN(name, text + 2, len - 2)) - continue; - } else if (STRNEQLEN(text, "--", len)) { - return NULL; - } - while (opt) { if (STREQ(opt->def->name, name) && opt->def->type != VSH_OT_ARGV) { exists = true; @@ -2790,14 +2779,15 @@ vshReadlineParse(const char *text, int state) } } - /* For string list returned by completer we have to do - * filtering based on @text because completer returns all - * possible strings. */ - if (vshCompleterFilter(&completer_list, text) < 0 || - virStringListMerge(&list, &completer_list) < 0) { + if (virStringListMerge(&list, &completer_list) < 0) goto cleanup; - } } + + /* For string list returned by completers we have to do + * filtering based on @text because completers returns all + * possible strings. */ + if (vshCompleterFilter(&list, text) < 0) + goto cleanup; } } -- 2.26.2
On Thu, 4 Feb 2021 15:13:33 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
This is what we do for completer callbacks: we let them generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix).
The same algorithm is implemented in vshReadlineOptionsGenerator() even though a bit differently. There is no need to have the same code twice.
I think this might be clearer stated a bit differently. For example, if I'm understanding correctly, a suggested alternate commit message: Completer callbacks generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix). In contrast, vshReadlineOptionsGenerator() does some internal filtering and only generates completions that match a given prefix. Rather than treating these scenarios differently, simply generate all possible options and filter them all at the end. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index f83b2a95a9..5f082f1a35 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2607,12 +2607,11 @@ vshReadlineCommandGenerator(const char *text) }
static char ** -vshReadlineOptionsGenerator(const char *text, +vshReadlineOptionsGenerator(const char *text G_GNUC_UNUSED, const vshCmdDef *cmd, vshCmd *last) { size_t list_index = 0; - size_t len = strlen(text); size_t ret_size = 0; g_auto(GStrv) ret = NULL;
@@ -2631,16 +2630,6 @@ vshReadlineOptionsGenerator(const char *text, if (cmd->opts[list_index].type == VSH_OT_ALIAS) continue;
- if (len > 2) { - /* provide auto-complete only when the text starts with -- */ - if (STRNEQLEN(text, "--", 2)) - return NULL; - if (STRNEQLEN(name, text + 2, len - 2)) - continue; - } else if (STRNEQLEN(text, "--", len)) { - return NULL; - } - while (opt) { if (STREQ(opt->def->name, name) && opt->def->type != VSH_OT_ARGV) { exists = true; @@ -2790,14 +2779,15 @@ vshReadlineParse(const char *text, int state) } }
- /* For string list returned by completer we have to do - * filtering based on @text because completer returns all - * possible strings. */ - if (vshCompleterFilter(&completer_list, text) < 0 || - virStringListMerge(&list, &completer_list) < 0) { + if (virStringListMerge(&list, &completer_list) < 0) goto cleanup; - } } + + /* For string list returned by completers we have to do + * filtering based on @text because completers returns all + * possible strings. */ + if (vshCompleterFilter(&list, text) < 0) + goto cleanup; } }
On 2/9/21 5:00 PM, Jonathon Jongsma wrote:
On Thu, 4 Feb 2021 15:13:33 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
This is what we do for completer callbacks: we let them generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix).
The same algorithm is implemented in vshReadlineOptionsGenerator() even though a bit differently. There is no need to have the same code twice.
I think this might be clearer stated a bit differently. For example, if I'm understanding correctly, a suggested alternate commit message:
Completer callbacks generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix).
In contrast, vshReadlineOptionsGenerator() does some internal filtering and only generates completions that match a given prefix. Rather than treating these scenarios differently, simply generate all possible options and filter them all at the end.
Yup, this sounds way better. Thanks! Michal
This is what we do for completer callbacks: we let them generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix). The same algorithm is implemented in vshReadlineCommandGenerator(). There is no need to have the same code twice. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 5f082f1a35..e0dbda04c8 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2561,11 +2561,10 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, * * Generator function for command completion. * - * Returns a string list of commands with @text prefix, - * NULL if there's no such command. + * Returns a string list of all commands, or NULL on failure. */ static char ** -vshReadlineCommandGenerator(const char *text) +vshReadlineCommandGenerator(const char *text G_GNUC_UNUSED) { size_t grp_list_index = 0, cmd_list_index = 0; const char *name; @@ -2587,15 +2586,13 @@ vshReadlineCommandGenerator(const char *text) if (cmds[cmd_list_index++].flags & VSH_CMD_FLAG_ALIAS) continue; - if (STRPREFIX(name, text)) { - if (VIR_REALLOC_N(ret, ret_size + 2) < 0) - return NULL; + if (VIR_REALLOC_N(ret, ret_size + 2) < 0) + return NULL; - ret[ret_size] = g_strdup(name); - ret_size++; - /* Terminate the string list properly. */ - ret[ret_size] = NULL; - } + ret[ret_size] = g_strdup(name); + ret_size++; + /* Terminate the string list properly. */ + ret[ret_size] = NULL; } } else { cmd_list_index = 0; @@ -2782,13 +2779,13 @@ vshReadlineParse(const char *text, int state) if (virStringListMerge(&list, &completer_list) < 0) goto cleanup; } - - /* For string list returned by completers we have to do - * filtering based on @text because completers returns all - * possible strings. */ - if (vshCompleterFilter(&list, text) < 0) - goto cleanup; } + + /* For string list returned by completers we have to do + * filtering based on @text because completers returns all + * possible strings. */ + if (vshCompleterFilter(&list, text) < 0) + goto cleanup; } if (list) { -- 2.26.2
On Thu, 4 Feb 2021 15:13:34 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
This is what we do for completer callbacks: we let them generate all possible outputs ignoring any partial input (e.g. prefix of a domain name) and then use vshCompleterFilter() to filter out those strings which don't fit the partial input (prefix).
The same algorithm is implemented in vshReadlineCommandGenerator(). There is no need to have the same code twice.
Similar comment as on previous patch Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 5f082f1a35..e0dbda04c8 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2561,11 +2561,10 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, * * Generator function for command completion. * - * Returns a string list of commands with @text prefix, - * NULL if there's no such command. + * Returns a string list of all commands, or NULL on failure. */ static char ** -vshReadlineCommandGenerator(const char *text) +vshReadlineCommandGenerator(const char *text G_GNUC_UNUSED) { size_t grp_list_index = 0, cmd_list_index = 0; const char *name; @@ -2587,15 +2586,13 @@ vshReadlineCommandGenerator(const char *text) if (cmds[cmd_list_index++].flags & VSH_CMD_FLAG_ALIAS) continue;
- if (STRPREFIX(name, text)) { - if (VIR_REALLOC_N(ret, ret_size + 2) < 0) - return NULL; + if (VIR_REALLOC_N(ret, ret_size + 2) < 0) + return NULL;
- ret[ret_size] = g_strdup(name); - ret_size++; - /* Terminate the string list properly. */ - ret[ret_size] = NULL; - } + ret[ret_size] = g_strdup(name); + ret_size++; + /* Terminate the string list properly. */ + ret[ret_size] = NULL; } } else { cmd_list_index = 0; @@ -2782,13 +2779,13 @@ vshReadlineParse(const char *text, int state) if (virStringListMerge(&list, &completer_list) < 0) goto cleanup; } - - /* For string list returned by completers we have to do - * filtering based on @text because completers returns all - * possible strings. */ - if (vshCompleterFilter(&list, text) < 0) - goto cleanup; } + + /* For string list returned by completers we have to do + * filtering based on @text because completers returns all + * possible strings. */ + if (vshCompleterFilter(&list, text) < 0) + goto cleanup; }
if (list) {
The way we currently call completer callbacks is that if we've found --option that user wants to complete value for and it has callback set then the callback is called. And just before that, if no --option to have the value completed is found or is found and is of boolean type then a list of --option is generated (for given command). But these two conditions can never be true at the same time because boolean type of --options do not accept values. Therefore the calling of completer callback can be promoted onto the same level as the --option list generation. This means that merging of two lists can be dropped to and completer callback can store its retval directly into @list (but as shown earlier one of the string lists to merge is always empty). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 44 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index e0dbda04c8..0d3c4fd7c7 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2750,34 +2750,28 @@ vshReadlineParse(const char *text, int state) if (!cmd) { list = vshReadlineCommandGenerator(text); - } else { - if (!opt || opt->type == VSH_OT_BOOL) - list = vshReadlineOptionsGenerator(text, cmd, partial); + } else if (!opt || opt->type == VSH_OT_BOOL) { + list = vshReadlineOptionsGenerator(text, cmd, partial); + } else if (opt && opt->completer) { + list = opt->completer(autoCompleteOpaque, + partial, + opt->completer_flags); + } - if (opt && opt->completer) { - g_auto(GStrv) completer_list = opt->completer(autoCompleteOpaque, - partial, - opt->completer_flags); + /* Escape completions, if needed (i.e. argument + * we are completing wasn't started with a quote + * character). This also enables filtering done + * below to work properly. */ + if (list && + !rl_completion_quote_character) { + size_t i; - /* Escape completions, if needed (i.e. argument - * we are 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; list[i]; i++) { + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - 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); - } - } - - if (virStringListMerge(&list, &completer_list) < 0) - goto cleanup; + virBufferEscape(&buf, '\\', " ", "%s", list[i]); + VIR_FREE(list[i]); + list[i] = virBufferContentAndReset(&buf); } } -- 2.26.2
Firstly, move variable declarations into the inner most block they are used. Secondly, use for() loop instead of while so that we don't have to advance loop counter explicitly on 'continue'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 0d3c4fd7c7..3650b7130a 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2566,37 +2566,30 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, static char ** vshReadlineCommandGenerator(const char *text G_GNUC_UNUSED) { - size_t grp_list_index = 0, cmd_list_index = 0; - const char *name; + size_t grp_list_index = 0; const vshCmdGrp *grp; - const vshCmdDef *cmds; size_t ret_size = 0; g_auto(GStrv) ret = NULL; grp = cmdGroups; - /* Return the next name which partially matches from the - * command list. - */ - while (grp[grp_list_index].name) { - cmds = grp[grp_list_index].commands; + for (grp_list_index = 0; grp[grp_list_index].name; grp_list_index++) { + const vshCmdDef *cmds = grp[grp_list_index].commands; + size_t cmd_list_index; - if (cmds[cmd_list_index].name) { - while ((name = cmds[cmd_list_index].name)) { - if (cmds[cmd_list_index++].flags & VSH_CMD_FLAG_ALIAS) - continue; + for (cmd_list_index = 0; cmds[cmd_list_index].name; cmd_list_index++) { + const char *name = cmds[cmd_list_index].name; - if (VIR_REALLOC_N(ret, ret_size + 2) < 0) - return NULL; + if (cmds[cmd_list_index].flags & VSH_CMD_FLAG_ALIAS) + continue; - ret[ret_size] = g_strdup(name); - ret_size++; - /* Terminate the string list properly. */ - ret[ret_size] = NULL; - } - } else { - cmd_list_index = 0; - grp_list_index++; + if (VIR_REALLOC_N(ret, ret_size + 2) < 0) + return NULL; + + ret[ret_size] = g_strdup(name); + ret_size++; + /* Terminate the string list properly. */ + ret[ret_size] = NULL; } } -- 2.26.2
After previous patches neither vshReadlineCommandGenerator() nor vshReadlineOptionsGenerator() use prefix that user wants to complete. The argument is marked as unused in both functions. Drop it then. 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 3650b7130a..708e8b6f86 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2557,14 +2557,13 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, /** * vshReadlineCommandGenerator: - * @text: optional command prefix * * Generator function for command completion. * * Returns a string list of all commands, or NULL on failure. */ static char ** -vshReadlineCommandGenerator(const char *text G_GNUC_UNUSED) +vshReadlineCommandGenerator(void) { size_t grp_list_index = 0; const vshCmdGrp *grp; @@ -2597,8 +2596,7 @@ vshReadlineCommandGenerator(const char *text G_GNUC_UNUSED) } static char ** -vshReadlineOptionsGenerator(const char *text G_GNUC_UNUSED, - const vshCmdDef *cmd, +vshReadlineOptionsGenerator(const vshCmdDef *cmd, vshCmd *last) { size_t list_index = 0; @@ -2742,9 +2740,9 @@ vshReadlineParse(const char *text, int state) opt = vshReadlineCommandFindOpt(partial); if (!cmd) { - list = vshReadlineCommandGenerator(text); + list = vshReadlineCommandGenerator(); } else if (!opt || opt->type == VSH_OT_BOOL) { - list = vshReadlineOptionsGenerator(text, cmd, partial); + list = vshReadlineOptionsGenerator(cmd, partial); } else if (opt && opt->completer) { list = opt->completer(autoCompleteOpaque, partial, -- 2.26.2
On Thu, 4 Feb 2021 15:13:25 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
There is no functional change to completion. No user visible change, nor to a developer who writes a completer callback. Some code deduplication and code cleanup and adapting code to 2021.
Michal Prívozník (12): lib: Substitute some STREQLEN with STRPREFIX vsh: Don't put VSH_OT_ALIAS onto list of completions vsh: Use g_auto(GStrv) to free string list returned by completer callback vsh: Accept NULL @list in vshCompleterFilter() vsh: Prefer g_strdup_printf() over g_snprintf() in vshReadlineOptionsGenerator() vsh: Use g_auto() for string lists returned in readline command/options generators vsh: Rewrite opt->type check in vshReadlineParse() vsh: Deduplicate filtering in vshReadlineOptionsGenerator() vsh: Deduplicate filtering in vshReadlineCommandGenerator() vsh: Simplify condition for calling completer callback vsh: Rework vshReadlineCommandGenerator() vsh: Drop unused @text arg from readline generators
src/libxl/xen_xl.c | 2 +- src/util/vircgroupv2.c | 10 +-- tools/vsh.c | 152 ++++++++++++++++------------------------- 3 files changed, 63 insertions(+), 101 deletions(-)
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
On 2/9/21 5:42 PM, Jonathon Jongsma wrote:
On Thu, 4 Feb 2021 15:13:25 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
There is no functional change to completion. No user visible change, nor to a developer who writes a completer callback. Some code deduplication and code cleanup and adapting code to 2021.
Michal Prívozník (12): lib: Substitute some STREQLEN with STRPREFIX vsh: Don't put VSH_OT_ALIAS onto list of completions vsh: Use g_auto(GStrv) to free string list returned by completer callback vsh: Accept NULL @list in vshCompleterFilter() vsh: Prefer g_strdup_printf() over g_snprintf() in vshReadlineOptionsGenerator() vsh: Use g_auto() for string lists returned in readline command/options generators vsh: Rewrite opt->type check in vshReadlineParse() vsh: Deduplicate filtering in vshReadlineOptionsGenerator() vsh: Deduplicate filtering in vshReadlineCommandGenerator() vsh: Simplify condition for calling completer callback vsh: Rework vshReadlineCommandGenerator() vsh: Drop unused @text arg from readline generators
src/libxl/xen_xl.c | 2 +- src/util/vircgroupv2.c | 10 +-- tools/vsh.c | 152 ++++++++++++++++------------------------- 3 files changed, 63 insertions(+), 101 deletions(-)
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Thanks, fixed those two commit messages and pushed. Michal
participants (2)
-
Jonathon Jongsma -
Michal Privoznik