Re: [libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code

On Mon, Oct 10, 2016 at 11:42:16 -0400, John Ferlan wrote: > 1. Move the declaration of const vshCmdDef *help - it should be at the > top of the "if" rather than in the middle. > > 2. Change a comparison from && to || - without doing so we could core Same comment as in 3/7 on 'core' not being the appropriate term. > on commands like 'virsh list' which would allow completion of some > non -- option based on whatever was found in the current working > directory and then as soon as that was completed, the next <tab> > would core since "opt" would be returned as NULL, but the check aaand again.

> was dereferencing "&& opt->type" > > 3. Before dereferencing opt->completer, be sure opt isn't NULL. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > tools/vsh.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tools/vsh.c b/tools/vsh.c > index 420eb79..9558dad 100644 > --- a/tools/vsh.c > +++ b/tools/vsh.c > @@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) > /* if we encountered --help, replace parsed command with > * 'help <cmdname>' */ > for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) { > + const vshCmdDef *help; > if (STRNEQ(tmpopt->def->name, "help")) > continue; > > - const vshCmdDef *help = vshCmddefSearch("help"); > + help = vshCmddefSearch("help"); > vshCommandOptFree(first); > first = vshMalloc(ctl, sizeof(vshCmdOpt)); > first->def = help->opts; > @@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state) > * Try to find the default option. > */ > if (!(opt = vshCmddefGetData(cmd, &opts_need_arg, &opts_seen)) > - && opt->type == VSH_OT_BOOL) > + || opt->type == VSH_OT_BOOL) > goto error; > opt_exists = true; > opts_need_arg = const_opts_need_arg; > @@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state) > res = vshReadlineCommandGenerator(sanitized_text, state); > } else if (opts_filled && !non_bool_opt_exists) { > res = vshReadlineOptionsGenerator(sanitized_text, state, cmd); > - } else if (non_bool_opt_exists && data_complete && opt->completer) { > + } else if (non_bool_opt_exists && data_complete && opt && opt->completer) { > if (!completed_list) > completed_list = opt->completer(autoCompleteOpaque, > opt->completer_flags); This function makes my eyes bleed.