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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list