On 08/20/2013 02:02 PM, Tomas Meszaros wrote:
Your commit message should give a brief summary of your change and why
it is important ("Add vshDomainCompleter" is the what, but you didn't
spell out the why - something along the lines of "future patches will
use this to improve readline completion").
> * global variable __my_conn renamed to vshConn
> * @name is now const char *
> * label cleanup renamed to error
These three lines are a changelog from your v1 patch, and not the "why"
you are making the change. In fact, since this patch does not mention
"__my_conn", no one will understand why you mentioned it at all in the
commit message. It is information that belongs better...
> ---
...here, after the --- separator, which is where 'git am' interprets the
break between the "why" of the commit message that belongs in git
history, vs. the "help" that is useful to list reviewers now, but has no
bearing on a future person reading git history.
> +
> + names = vshMalloc(NULL, sizeof(char *) * (ndomains + 1));
> +
> + if (!names)
> + return NULL;
Dead code. names is non-null, thanks to vshMalloc.
> +error:
> + for (i = 0; names[i]; i++)
> + VIR_FREE(names[i]);
> + VIR_FREE(names);
Simpler to use virStringFreeList(names) here, instead of inlining the
iteration.
> @@ -3510,6 +3557,7 @@ main(int argc, char **argv)
> ctl->debug = VSH_DEBUG_DEFAULT;
> ctl->escapeChar = "^]"; /* Same default as telnet */
>
> + vshConn = &ctl->conn;
>
> if (!setlocale(LC_ALL, "")) {
> perror("setlocale");
> @@ -3571,6 +3619,11 @@ main(int argc, char **argv)
> exit(EXIT_FAILURE);
> }
>
> + /* Need to connect immediately after start in order to provide
> + * autocompletion for the very first command.
> + */
> + vshReconnect(ctl);
> +
I'm not a fan of this. We worked hard to get some commands (like 'virsh
echo') to NOT need a connection, and this would be undoing that work. I
think we should NOT autoconnect UNTIL we reach the point that completion
is being attempted on something where a connection is needed to get the
completer list, rather than this code which autoconnects regardless of
whether we need completion. Having 'virsh echo' avoid an autoconnect is
useful - it can be used to prove whether virsh itself is working or has
a problem such as a missing .so, without complicating the diagnosis with
a question of whether it is a bad default connection to blame.
Also, at some point in your series, you will want to introduce a new
virsh command whose job is to perform the completion of its arguments.
That way, we can have auto-completion both within the virsh interactive
shell, and from bash, while only having to maintain it in one place.
That is, if I type:
$ virsh
virsh> start a<TAB>
I'm using the interactive virsh completion to list domains starting with
a; meanwhile, if I type:
$ virsh start a<TAB>
then the bash completion handler would call:
$ virsh complete start a
under the hood, so that virsh outputs the list of names that can be fed
back to the bash completion hooks. The bash completion routine is then
dead simple - forward all completions to virsh itself. I don't want to
have to teach bash completion routines which commands expect a domain,
nor how to generate its own domain listing, when virsh already has that
code.
--
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