[libvirt] [PATCHv3 3/6] virsh: Improve readline generators and readline completion
Eric Blake
eblake at redhat.com
Mon Aug 26 17:54:47 UTC 2013
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
> New completion generators responsible for advances command
> and command options completions.
Not sure if you meant s/advances/advanced/ ?
It might help to add a link to
http://cnswww.cns.cwru.edu/php/chet/readline/readline.html#SEC44, which
is documentation on how readline custom completers work, as part of the
commit message and/or in the code where you are installing custom
completers.
>
> .gnulib | 2 +-
> tools/virsh.c | 386 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 365 insertions(+), 23 deletions(-)
Oops, the change to .gnulib should NOT be here. You can fix it by
rebasing this series (git rebase -i, then change 'pick' to 'edit' for
this patch), and while on this patch, do 'git checkout HEAD^ .gnulib',
then 'make' (which should automatically rerun ./autogen.sh and thus
cover any fallout from the change to the submodule).
This feels like a pretty big patch. It adds a number of new functions
all at once; does it make sense to split this into multiple smaller
patches, focusing on one thing at a time?
> +++ b/tools/virsh.c
> @@ -2515,6 +2515,25 @@ vshCloseLogFile(vshControl *ctl)
> * -----------------
> */
>
> +static const vshCmdDef *
> +vshDetermineCommandName(void)
> +{
> + const vshCmdDef *cmd = NULL;
> + char *p;
> + char *cmdname;
> +
> + if (!(p = strchr(rl_line_buffer, ' ')))
> + return NULL;
> +
> + cmdname = vshMalloc(NULL, (p - rl_line_buffer) + 1);
> + memcpy(cmdname, rl_line_buffer, p - rl_line_buffer);
Why not VIR_STRNDUP? Oh...
> +
> + cmd = vshCmddefSearch(cmdname);
> + VIR_FREE(cmdname);
> +
> + return cmd;
> +}
> +
> /*
> * Generator function for command completion. STATE lets us
> * know whether to start from scratch; without any state
> @@ -2562,25 +2581,14 @@ vshReadlineCommandGenerator(const char *text, int state)
> static char *
> vshReadlineOptionsGenerator(const char *text, int state)
> {
> - static int list_index, len;
> static const vshCmdDef *cmd = NULL;
> + static int list_index, len;
> const char *name;
>
> if (!state) {
> - /* determine command name */
> - char *p;
> - char *cmdname;
> -
> - if (!(p = strchr(rl_line_buffer, ' ')))
> - return NULL;
> -
> - cmdname = vshCalloc(NULL, (p - rl_line_buffer) + 1, 1);
> - memcpy(cmdname, rl_line_buffer, p - rl_line_buffer);
...because this is a refactoring of pre-existing code.
> -
> - cmd = vshCmddefSearch(cmdname);
> + cmd = vshDetermineCommandName();
> list_index = 0;
> len = strlen(text);
> - VIR_FREE(cmdname);
> }
>
> if (!cmd)
> @@ -2612,22 +2620,356 @@ vshReadlineOptionsGenerator(const char *text, int state)
> return NULL;
> }
>
My summary from what I gathered after reading the readline programmer's
interface: any time a completion is attempted, the generator function
will be called repeatedly until it returns NULL; the first call has
state==0 and all subsequent calls have non-negative (but otherwise
unspecified) state. Each non-NULL return must be a malloc'd string that
serves as a possible completion of 'text'.
> +/*
> + * Generator function for command completion, but unlike
> + * the vshReadlineCommandGenerator which completes command name, this function
> + * provides more advanced completion for commands by calling specific command
> + * completers (e.g. vshDomainCompleter).
> + */
> +static char *
> +vshReadlineCommandCompletionGenerator(const char *text, int state)
> +{
> + static const vshCmdDef *cmd = NULL;
Explicit NULL initialization is not required - all 'static' variables
are implicitly zero-initialized.
> + static int list_index, len;
We maintain static state between functions (yuck - readline is not
thread-friendly - thankfully, we are only ever using completions from
the cli-parsing thread, so I guess we can get away with it).
> + char **completed_names = NULL;
> + char *name;
> +
> + if (!state) {
> + cmd = vshDetermineCommandName();
> + list_index = 0;
> + len = strlen(text);
> + }
So on the first call, we determine which command we are completing, and
cache the length of 'text' being completed.
> +
> + if (!cmd)
> + return NULL;
If we aren't completing a command, we're done (fall back to readline's
default completer).
> +
> + if (!cmd->completer)
> + return NULL;
If the command doesn't have a specific completer, we're done (fall back
to readline's default completer).
> +
> + completed_names = cmd->completer(cmd->completer_flags);
Malloc a complete list of names. Ouch - that means we malloc the list
on EVERY iteration of this function, even though we know this function
is called multiple times during one completion attempt. Really, we
should only malloc the list when state == 0, and refer back to that list
on all other calls. Which means the list of completed_names should be
static across calls.
> +
> + if (!completed_names)
> + return NULL;
> +
> + while ((name = completed_names[list_index])) {
For each name that the helper completer returned (but starting at the
point that we remembered from last time around - all the more reason
that we should ALSO remember the list from last time around, rather than
recomputing the list)...
> + char *res;
> + list_index++;
> +
> + if (STRNEQLEN(name, text, len))
> + /* Skip irrelevant names */
> + continue;
...if it doesn't match the text we were given, skip it...
> +
> + res = vshStrdup(NULL, name);
> + VIR_FREE(name);
Yuck. Why are we malloc'ing a copy and then freeing the original? Why
not just do transfer semantics (completed_name[list_index] = NULL, then
return name as is, without going through another strdup)?
> + return res;
> + }
> + VIR_FREE(completed_names);
Oops - this frees the list of names, but does NOT free the names
contained within the list. If ALL such names matched text, then you
avoided a leak; but if 'text' is not empty, then you are leaking
everywhere that you did a 'continue' in the loop above.
Oops - this code is only reached if you return NULL, but completed_names
was allocated on every entry (again, my argument is that completed_names
should be static, so that it is collected when state==0, and freed when
returning NULL, so that all other calls in between reuse the list).
> +
> + return NULL;
> +}
> +
> +/*
> + * Generator function for command option completion. Provides advances
s/advances/advanced/
> + * completion for command options.
> + */
> +static char *
> +vshReadlineOptionsCompletionGenerator(const char *text ATTRIBUTE_UNUSED,
> + int state ATTRIBUTE_UNUSED)
> +{
> + static const vshCmdDef *cmd = NULL;
> + static const vshCmdOptDef *opt = NULL;
> + static int list_index, len;
> + unsigned long int opt_index = 0;
> + size_t i;
> + char **completed_names = NULL;
Similar problems about this list not being static between calls.
> +
> + for (i = 0; cmd->opts[i].name; i++) {
> + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name)) &&
Are you sure strstr() is right? Wouldn't STRPREFIX be more appropriate?
That is, you only want to complete when you share a common prefix, not
when the text is a substring anywhere within a longer name.
> +
> +/*
> + * Returns current valid command name present in the rl_line_buffer.
> + */
> +static char *
> +vshCurrentCmd(void)
> +{
> + const char *name;
> + const vshCmdGrp *grp;
> + const vshCmdDef *cmds;
> + size_t grp_list_index, cmd_list_index;
> + char *found_cmd = NULL;
> + char *rl_copy = NULL;
Please do NOT name your local variable rl_copy. rl_* is reserved for
readline entry points, and mixing namespaces will only cause confusion
for future readers of the code.
> + char *pch;
How often is this function going to be called? Is it something where
you should cache the results in something a bit longer-lasting? (Again,
I'm not too terribly fond of how thread-unsafe readline is, but as long
as it is only used by a single thread doing command-line parsing, we
might as well be efficient in our interactions with it)
> +
> + grp_list_index = 0;
> + cmd_list_index = 0;
> + grp = cmdGroups;
> +
> + while (grp[grp_list_index].name) {
> + cmds = grp[grp_list_index].commands;
> +
> + if (cmds[cmd_list_index].name) {
> + while ((name = cmds[cmd_list_index].name)) {
> + cmd_list_index++;
> +
> + if (VIR_STRDUP(rl_copy, rl_line_buffer) < 0)
> + return NULL;
> +
> + char *saveptr;
> + pch = strtok_r(rl_copy, " ", &saveptr);
Does this work correctly with virsh's quoting rules, which are there to
allow for quoting arguments that contain a space?
> +
> +/*
> + * Returns current valid command option name present in the rl_line_buffer.
> + */
> +static char *
> +vshCurrentOpt(const char *cmd_name)
> +{
> + const vshCmdDef *cmd = NULL;
> + const char *name;
> + unsigned long int opt_index = 0;
> + size_t cmd_opt_list_index;
> + char *found_opt = NULL;
> + char *ptr = NULL;
> +
> + if (!cmd_name)
> + return NULL;
> +
> + cmd = vshCmddefSearch(cmd_name);
> +
> + if (!cmd)
> + return NULL;
> +
> + if (!cmd->opts)
> + return NULL;
> +
> + cmd_opt_list_index = 0;
> +
> + while ((name = cmd->opts[cmd_opt_list_index].name)) {
> + cmd_opt_list_index++;
> +
> + if ((ptr = strstr(rl_line_buffer, name))) {
Again, I'm betting that strstr() is wrong.
> + if (opt_index < (ptr - rl_line_buffer)) {
> + opt_index = ptr - rl_line_buffer;
> + if (VIR_STRDUP(found_opt, name) < 0)
> + return NULL;
> + }
> + }
> + }
> +
> + return found_opt;
> +}
> +
> +/*
> + * Returns name of the command completion currently present in the rl_line_buffer.
> + */
> +static char*
> +vshCurrentCmdCom(const char *cmd_name)
> +{
> + const vshCmdDef *cmd = NULL;
> + size_t i;
> + char **completed_names = NULL;
> + char *found_cmd_com = NULL;
> +
> + if (!cmd_name)
> + return NULL;
> +
> + cmd = vshCmddefSearch(cmd_name);
> +
> + if (!cmd)
> + return NULL;
> +
> + if (!cmd->completer)
> + return NULL;
> +
> + completed_names = cmd->completer(cmd->completer_flags);
> +
> + if (!completed_names)
> + return NULL;
> +
> + for (i = 0; completed_names[i]; i++) {
> + if (strstr(rl_line_buffer, completed_names[i])) {
Another suspicious strstr.
> + if (VIR_STRDUP(found_cmd_com, completed_names[i]) < 0)
> + return NULL;
> + }
> + }
> +
> + return found_cmd_com;
> +}
> +
> +/*
> + * Returns name of the option completion currently present in the rl_line_buffer.
> + */
> +static char *
> +vshCurrentOptCom(const char *cmd_name)
> +{
> + const vshCmdDef *cmd = NULL;
> + const vshCmdOptDef *opt = NULL;
> + unsigned long int opt_index = 0;
> + size_t i;
> + char **completed_names = NULL;
> + char *found_opt_com = NULL;
> + char *ptr = NULL;
> +
> + if (!cmd_name)
> + return NULL;
> +
> + cmd = vshCmddefSearch(cmd_name);
> +
> + if (!cmd)
> + return NULL;
> +
> + if (!cmd->opts)
> + return NULL;
> +
> + for (i = 0; cmd->opts[i].name; i++) {
> + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name))) {
Another suspicious strstr.
> + if (opt_index < (ptr - rl_line_buffer)) {
> + opt_index = ptr - rl_line_buffer;
> + opt = &cmd->opts[i];
> + }
> + }
> + }
> +
> + if (!opt)
> + return NULL;
> +
> + if (!opt->completer)
> + return NULL;
> +
> + completed_names = opt->completer(opt->completer_flags);
> +
> + if (!completed_names)
> + return NULL;
> +
> + for (i = 0; completed_names[i]; i++) {
> + if (strstr(rl_line_buffer, completed_names[i])) {
and another.
> + if (VIR_STRDUP(found_opt_com, completed_names[i]) < 0)
> + return NULL;
> + }
> + }
> +
> + return found_opt_com;
> +}
> +
> static char **
> -vshReadlineCompletion(const char *text, int start,
> - int end ATTRIBUTE_UNUSED)
> +vshReadlineCompletion(const char *text, int start, int end ATTRIBUTE_UNUSED)
Why the spurious reformat?
> {
> - char **matches = (char **) NULL;
> + const char *cmd = vshCurrentCmd();
> + const char *cmd_com = vshCurrentCmdCom(cmd);
> + const char *opt = vshCurrentOpt(cmd);
> + const char *opt_com = vshCurrentOptCom(cmd);
> + char **matches = (char **)NULL;
>
> - if (start == 0)
> - /* command name generator */
> + if (start == 0) {
> + /* Command name generator. */
> matches = rl_completion_matches(text, vshReadlineCommandGenerator);
> - else
> - /* commands options */
> - matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
> + } else {
> + /* Command completer, commands options and commands options completer
> + * generators.
> + */
> + if (strstr(text, "-")) {
Again, strstr() is probably dead wrong here.
> + /* When user wants to see options. */
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
> + } else if (!cmd_com && !opt && !opt_com) {
> + matches = rl_completion_matches(text, vshReadlineCommandCompletionGenerator);
> + if (!matches)
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
I'm not sure this is quite right. Remember, in my example of 2/6, if
the user types 'vol-key <TAB>', the completion should list the UNION of
two different completions: all possible options (--vol and --pool, maybe
even --help) and all possible volume names that don't resemble options.
But I don't see the filtering that rejects a volume name beginning with
'-'.
> + } else if (cmd_com && !opt && !opt_com) {
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
> + } else if (cmd_com && opt && !opt_com) {
> + matches = rl_completion_matches(text, vshReadlineOptionsCompletionGenerator);
> + if (!matches)
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
> + } else if (!cmd_com && opt && !opt_com) {
> + matches = rl_completion_matches(text, vshReadlineCommandCompletionGenerator);
> + if (!matches)
> + matches = rl_completion_matches(text, vshReadlineOptionsCompletionGenerator);
> + if (!matches)
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
> + } else if (!cmd_com && opt && opt_com) {
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
> + } else if (cmd_com && opt && opt_com) {
> + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
This looks like a rat's nest of completion functions, especially, when
some of the clauses (like these last two) have identical contents (and
could be combined into one by using simply 'if (opt && opt_com)' instead
of basing off of !cmd_com/cmd_com). Are we sure this complexity is
necessary?
> + }
> + }
> +
> + VIR_FREE(cmd);
> + VIR_FREE(cmd_com);
> + VIR_FREE(opt);
> + VIR_FREE(opt_com);
> +
> return matches;
> }
>
> -
> static int
> vshReadlineInit(vshControl *ctl)
> {
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130826/dc59321f/attachment-0001.sig>
More information about the libvir-list
mailing list