Re: [libvirt] [PATCHv3 3/6] virsh: Improve readline generators and readline completion

On 08/26/2013 06:36 AM, Tomas Meszaros wrote: 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. 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? Why not VIR_STRNDUP? Oh... ...because this is a refactoring of pre-existing code. 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'. Explicit NULL initialization is not required - all 'static' variables are implicitly zero-initialized. 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). So on the first call, we determine which command we are completing, and cache the length of 'text' being completed. If we aren't completing a command, we're done (fall back to readline's default completer). If the command doesn't have a specific completer, we're done (fall back to readline's default completer). 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. 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)... ...if it doesn't match the text we were given, skip it... 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)? 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). 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; > + unsigned long int opt_index = 0; > + size_t i; > + char **completed_names = NULL; Similar problems about this list not being static between calls. 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. 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. 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) 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->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 (!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->opts) > + return NULL; > + > + for (i = 0; cmd->opts[i].name; i++) { > + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name))) { Another suspicious strstr. and another. Why the spurious reformat? Again, strstr() is probably dead wrong here. 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 '-'. 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? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org