Re: [libvirt] [PATCH 3/4] virsh: Create completer for commands related to domains

On 07/04/2011 02:41 AM, Michal Privoznik wrote: 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). 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. 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. 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. 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... 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. See my comments on 2/4 why I think this is the wrong place to be using the new completion function. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org