8:13 a.m.
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
8:13 a.m.
New subject: [PATCH 01/12] lib: Substitute some STREQLEN with STRPREFIX
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(a)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
8:13 a.m.
New subject: [PATCH 02/12] vsh: Don't put VSH_OT_ALIAS onto list of completions
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(a)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
8:13 a.m.
New subject: [PATCH 03/12] vsh: Use g_auto(GStrv) to free string list returned by completer callback
This saves us explicit call of g_strfreev() in error path.
Signed-off-by: Michal Privoznik <mprivozn(a)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
8:13 a.m.
New subject: [PATCH 04/12] vsh: Accept NULL @list in vshCompleterFilter()
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(a)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
8:13 a.m.
New subject: [PATCH 05/12] vsh: Prefer g_strdup_printf() over g_snprintf() in vshReadlineOptionsGenerator()
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(a)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
8:13 a.m.
New subject: [PATCH 06/12] vsh: Use g_auto() for string lists returned in readline command/options generators
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(a)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
8:13 a.m.
New subject: [PATCH 07/12] vsh: Rewrite opt->type check in vshReadlineParse()
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(a)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
8:13 a.m.
New subject: [PATCH 08/12] vsh: Deduplicate filtering in vshReadlineOptionsGenerator()
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(a)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
10 a.m.
New subject: [PATCH 08/12] vsh: Deduplicate filtering in vshReadlineOptionsGenerator()
On Thu, 4 Feb 2021 15:13:33 +0100
Michal Privoznik <mprivozn(a)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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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;
}
}
4:54 a.m.
New subject: [PATCH 08/12] vsh: Deduplicate filtering in vshReadlineOptionsGenerator()
On 2/9/21 5:00 PM, Jonathon Jongsma wrote:
On Thu, 4 Feb 2021 15:13:33 +0100
Michal Privoznik <mprivozn(a)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
8:13 a.m.
New subject: [PATCH 09/12] vsh: Deduplicate filtering in vshReadlineCommandGenerator()
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(a)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
10:30 a.m.
New subject: [PATCH 09/12] vsh: Deduplicate filtering in vshReadlineCommandGenerator()
On Thu, 4 Feb 2021 15:13:34 +0100
Michal Privoznik <mprivozn(a)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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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) {
8:13 a.m.
New subject: [PATCH 10/12] vsh: Simplify condition for calling completer callback
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(a)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
8:13 a.m.
New subject: [PATCH 11/12] vsh: Rework vshReadlineCommandGenerator()
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(a)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
8:13 a.m.
New subject: [PATCH 12/12] vsh: Drop unused @text arg from readline generators
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(a)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
10:42 a.m.
On Thu, 4 Feb 2021 15:13:25 +0100
Michal Privoznik <mprivozn(a)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(a)redhat.com>
4:54 a.m.
On 2/9/21 5:42 PM, Jonathon Jongsma wrote:
On Thu, 4 Feb 2021 15:13:25 +0100
Michal Privoznik <mprivozn(a)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(a)redhat.com>
Thanks, fixed those two commit messages and pushed.
Michal
1371
days inactive
1377
days old
17 comments
2 participants
participants (2)
-
Jonathon Jongsma -
Michal Privoznik