On 07/04/2011 02:41 AM, Michal Privoznik wrote:
> This completer allows selection of active and/or inactive domains so
> only domains in valid state for a command are listed, e.g. only inactive
> for 'start' command, active for 'shutdown' and so on.
> ---
> tools/virsh.c | 250 +++++++++++++++++++++++++++++++++++++++++++++------------
> 1 files changed, 199 insertions(+), 51 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 3dabb10..6a63363 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -600,6 +600,119 @@ vshReconnect(vshControl *ctl)
> }
>
> /* ---------------
> + * Completers
> + * ---------------
> + */
> +#define ACTIVE (1<<0)
> +#define INACTIVE (1<<1)
> +static char *
> +complt_listDomainsFlags(const char *text ATTRIBUTE_UNUSED, int state, unsigned int flags)
> +{
This function might still be useful, even if you change to having
per-option completers instead of per-command completers.
You _need_ to pay attention to 'text' - if I type:
start d<TAB>
then I want _only_ the domains whose names start with d, which you can
only know if you read 'text' to see that input left off with a starting
'd'. Also, if 'text' starts off at the beginning of a word, you want a
flag that says whether to include or filter out domain names that happen
to start with '-' (if I type
start --domain -<TAB>
then list only domains starting with dash, whereas if I type
start <TAB>
then listing a domain name starting with dash would be wrong because it
would be in a position that would be interpreted as an option rather
than a domain name).
> + static int len = 0;
> + static int index_active = 0;
> + static int index_inactive = 0;
> + static int maxid = 0;
> + static int *ids = NULL;
> + static int maxname = 0;
> + static char **names = NULL;
> + char *ret;
That's a lot of static variables. If we ever need to go thread-local,
it would be easier to create a single struct, and malloc a thread-local
instance of that struct. But for now this is probably okay; it's not as
severe as the patch 1/4 where going thread-local would affect all callers.
> +
> + /*
> + * TODO:
> + * If we are not connected, should we connect here
> + * or simply return NULL and thus not complete 'live data'?
> + */
> + if (!conn)
> + return NULL;
Hmm. In most cases, we can then reuse the connection. Besides, when I
type:
start d<TAB>
I don't want the behavior to depend on whether I have run a previous
command that opened a connection.
But in reality, maybe the thing to do is to return NULL here if conn is
not already established, and instead teach the generic completion that
it should autoconnect if attempting completion on a command that itself
is marked auto-connect. That is,
virsh help <TAB>
should _not_ open a connection, since cmdHelp is marked
VSH_CMD_FLAG_NOCONNECT, but:
virsh start <TAB> is fine opening the connection, since cmdStart would
have also opened the connection, and will happily reuse an already-open
connection.
Furthermore, this integrates well into the fact that we (eventually)
need to add a completion command:
virsh start d<TAB>
would be hooked to a bash completion function that calls:
virsh complete 'start d'
where the new cmdComplete is marked VSH_CMD_FLAG_NOCONNECT (that is, it
does not connect on its own), but which will auto-connect if it detects
that it is being used to generate the completion of some other command
that would normally autoconnect.
> +
> + if (!state) {
> + len = strlen(text);
> + maxid = 0;
> + maxname = 0;
> +
> + if (flags & ACTIVE) {
> + maxid = virConnectNumOfDomains(conn);
> + if (maxid < 0)
> + goto cleanup;
> + if (maxid) {
> + ids = vshMalloc(NULL, sizeof(int) * maxid);
> + if ((maxid = virConnectListDomains(conn, ids, maxid)) < 0)
> + goto cleanup;
> +
> + qsort(ids, maxid, sizeof(int), idsorter);
> + }
> + }
> + if (flags & INACTIVE) {
> + maxname = virConnectNumOfDefinedDomains(conn);
> + if (maxname < 0)
> + goto cleanup;
> + if (maxname) {
> + names = vshMalloc(NULL, sizeof(char *) * maxname);
> + if ((maxname =
> + virConnectListDefinedDomains(conn, names, maxname)) < 0)
> + goto cleanup;
> +
> + qsort(names, maxname, sizeof(char *), namesorter);
Why are we sorting here? Either readline sorts as well (and this is
redundant), or if we are generating per-option completions, then we
might be merging this return list into a larger list of other possible
completions, at which point we would want that overall list sorted by
the generic completion handler rather than just here.
> + }
> + }
> +
> + index_active = 0;
> + index_inactive = 0;
> + }
> +
> + while (index_active < maxid) {
> + virDomainPtr dom = virDomainLookupByID(conn, ids[index_active]);
> + index_active++;
> +
> + if (!dom)
> + continue;
> +
> + ret = (char *) virDomainGetName(dom);
> + if (STRNEQLEN(ret, text, len)) {
What happens if the domain disappears between the time that you gathered
the list and now that you call virDomainGetName()? STRNEQLEN is not
very forgiving of a NULL (ret) parameter. In that case, the best thing
to do would be just skipping that index, and going on to the next one.
It seems odd to me to be reusing 'ret' here with a cast to (char*), for
what is really 'const char *' data, especially since...
> + virDomainFree(dom);
> + continue;
> + }
> + ret = vshStrdup(NULL, ret);
here, you have to malloc ret into a real 'char *' data to meet the
rl_completion generator contract. It seems like using a secondary
declaration const char *name = virDomainGetName(dom) might be wiser.
> @@ -11615,84 +11728,119 @@ cleanup:
>
> static const vshCmdDef domManagementCmds[] = {
> {"attach-device", cmdAttachDevice, opts_attach_device,
> - info_attach_device, 0, NULL},
> + info_attach_device, 0, complt_listAllDomains},
See my comments on 2/4 why I think this is the wrong place to be using
the new completion function.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
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