[libvirt] [PATCHv3 4/6] virsh: Add vshDomainCompleter

[Eric Blake]

On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
> Function vshDomainCompler returns domains names which can be used
> by various virsh commands, for example:
> 
> virsh> start <TAB>
> fedora      domain_foo    domain_bar
> virsh> start f<TAB>
> virsh> start fedora
> 
> Michal Privoznik recommended to add global variable virConnectPtr *__my_conn

Stale name in the commit message.

> so we can get the list of domains from the virConnecTListAllDomains().
> 
> vshReconnect() is called before the first command is executed
> in order to provide autocompletion for the very first command.

Stale comment, now that you fixed it to delay until completion requires
a connection.

> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index b29b82a..0f30902 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1876,7 +1876,9 @@ const vshCmdDef domMonitoringCmds[] = {
>       .handler = cmdDomBlkError,
>       .opts = opts_domblkerror,
>       .info = info_domblkerror,
> -     .flags = 0
> +     .flags = 0,
> +     .completer = vshDomainCompleter,
> +     .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE

I'm still not sure I get why we need per-command completers; isn't
per-option completion sufficient (by associating the completer with the
--domain of each of these commands)?

> @@ -1888,7 +1890,10 @@ const vshCmdDef domMonitoringCmds[] = {
>       .handler = cmdDomblklist,
>       .opts = opts_domblklist,
>       .info = info_domblklist,
> -     .flags = 0
> +     .flags = 0,
> +     .completer = vshDomainCompleter,
> +     .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
> +                        VIR_CONNECT_LIST_DOMAINS_INACTIVE

Style: please use trailing comma after the last element of the struct,
so that future additions don't have to modify existing lines when adding
even more struct members down the road (throughout this patch, and
probably throughout the series).  Yeah, I know existing C99 initializers
for vshCmdInfo.dad and vshCmdOptDef.help aren't using trailing commas
consistently, so maybe that's worth a separate cleanup patch.  If we had
been following that style, you wouldn't be modifying the .flags lines.

> +++ b/tools/virsh.c
> @@ -88,6 +88,8 @@ static char *progname;
>  
>  static const vshCmdGrp cmdGroups[];
>  
> +vshControl *vshCtl;

This variable should probably be marked static, as I don't see it
referenced in any other file.

> +
>  /* Bypass header poison */
>  #undef strdup
>  
> @@ -317,6 +319,7 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED,
>  static void
>  vshReconnect(vshControl *ctl)
>  {
> +    printf("\n>>>\n");

Leftover debugging?

> @@ -2510,6 +2513,46 @@ vshCloseLogFile(vshControl *ctl)
>  
>  #ifdef USE_READLINE
>  
> +/* -------------
> + * Completers
> + * -------------
> + */
> +
> +char **
> +vshDomainCompleter(unsigned int flags)

Ouch - you have a link error if building without USE_READLINE, since the
use of this function was unconditional but its implementation is
conditional.  How much of this code can you hoist outside of
USE_READLINE?  If you can't, then please provide the #else half so that
at least things still link.

> +{
> +    virDomainPtr *domains;
> +    size_t i;
> +    char **names = NULL;
> +    int ndomains;
> +
> +    if (!vshCtl->conn)
> +        return NULL;
> +
> +    ndomains = virConnectListAllDomains(vshCtl->conn, &domains, flags);
> +
> +    if (ndomains < 0)
> +        return NULL;
> +
> +    names = vshMalloc(NULL, sizeof(char *) * (ndomains + 1));
> +

VIR_ALLOC_N seems nicer for the task.

> +    for (i = 0; i < ndomains; i++) {
> +        const char *name = virDomainGetName(domains[i]);
> +        if (VIR_STRDUP(names[i], name) < 0) {
> +            virDomainFree(domains[i]);
> +            goto error;
> +        }
> +        virDomainFree(domains[i]);
> +    }
> +    names[i] = NULL;
> +    VIR_FREE(domains);
> +    return names;
> +
> +error:

Memory leak - if you get here, you don't finish calling virDomainFree()
on the tail of the list, nor VIR_FREE(domains).  That cleanup needs to
be unconditional.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130826/f0e3fbb9/attachment-0001.sig>