[libvirt] [PATCH 5/5] virsh: add support for virConnectListAllDomains and clean up cmdList

[Eric Blake]

On 05/20/2012 09:56 AM, Peter Krempa wrote:
> This patch makes use of the newly added api virConnectListAllDomains()
> to list domains in virsh.
> 
> To enable fallback virsh now represents lists of domains using an

s/fallback/fallback,/

> internal structure vshDomainList. This structure contains the
> virDomainPtr list as provided by virConnectListAllDomains() and the
> count of domains in the list.
> 
> For backwards compatiblity I've added function vshDomList that tries to

s/compatiblity/compatibility/
s/added/added the/

> enumerate the domains using the new API and if the API is not supported

s/new API/new API,/

> falls back to the older approach with the two list functions.
> 
> This patch also adds helpers to filter domain lists provided by
> vshDomList().

Is it vshDomainList or vshDomList?  Can you reuse the filtering bits of
the public API when falling back to the older APIs?

> 
> As a last improvement I've cleaned up code that handles the "list"
> command using the newly added helper and filter functions.
> ---
>  tools/virsh.c |  423 +++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 273 insertions(+), 150 deletions(-)
> 

> +static int
> +namesorter(const void *a, const void *b)
> +{
> +    const char **sa = (const char**)a;
> +    const char **sb = (const char**)b;
> 
> -  if (*ia > *ib)
> -    return 1;
> -  else if (*ia < *ib)
> -    return -1;
> -  return 0;
> +    /* User visible sort, so we want locale-specific case comparison.  */
> +    return strcasecmp(*sa, *sb);

Hmm.  Pre-existing (since you're only floating namesorter higher up in
the file), but strcasecmp has undefined behavior in some locales where
not all byte sequences are valid characters.  It's quite painful to work
around strcasecmp failure inside a qsort (see the GPL module xmemcoll in
gnulib to see how horrible it is).  But since it's pre-existing and no
one has complained yet, I'm find leaving it as is.

> +}
> +
> +static int
> +domsorter(const void *a, const void *b)
> +{
> +    virDomainPtr *da = (virDomainPtr *) a;
> +    virDomainPtr *db = (virDomainPtr *) b;
> +    unsigned int ida = virDomainGetID(*da);
> +    unsigned int idb = virDomainGetID(*db);
> +    unsigned int err = (unsigned int) -1;

'err' is a misleading name.  Really, it is more like 'inactive'.  (Oh
wow - we return -1 for both inactive domains with no error set, and for
garbage in with a VIR_ERR_INVALID_DOMAIN error - but the latter case
shouldn't be happening because virsh shouldn't be feeding in garbage).

> +
> +    if (ida == err && idb == err) {
> +        if (virDomainGetName(*da) ||
> +            virDomainGetName(*db))

All domains have names; virDomainGetName can't fail unless you feed it
garbage, so this if will always occur,

> +                return strcasecmp(virDomainGetName(*da),
> +                                  virDomainGetName(*db));

and this is a wasted double-function call,

> +        else
> +            return 0;

and you will never get here if virsh is bug-free.

> +    }
> +
> +    if (ida != err && idb != err) {
> +        if (ida > idb)
> +            return 1;
> +        else if (ida < idb)
> +            return -1;
> +
> +        return 0;
> +    }
> +
> +    if (ida != err)
> +        return -1;
> +    else
> +        return 1;

Looks like a sane sort; you manage to preserve the old 'virsh list'
ordering of active before inactive.

> +static int
> +vshDomList(vshControl *ctl, vshDomainListPtr domlist)

Passing in a flags argument here will be important for using filtering
of the public API when available, and faking the filtering otherwise.

> +
> +    if ((ret = virConnectListAllDomains(ctl->conn,
> +                                        &(domlist->domains),
> +                                        -1, 0)) >= 0) {
> +        domlist->ndomains = ret;
> +        qsort(domlist->domains, domlist->ndomains,
> +              sizeof(virDomainPtr), domsorter);
> +        return ret;
> +    }
> +
> +    if ((err = virGetLastError()) &&
> +        err->code != VIR_ERR_NO_SUPPORT) {
> +        vshError(ctl, "%s", _("Failed to list domains"));
> +        goto cleanup;
> +    }
> +
> +    /* fall back to old method */
> +    virResetLastError();

Looks good through here.

> +
> +    if ((nids = virConnectNumOfDomains(ctl->conn)) < 0) {
> +        vshError(ctl, "%s", _("Failed to list active domains"));
> +        goto cleanup;
> +    }
> +
> +    if (nids) {
> +        ids = vshMalloc(ctl, sizeof(int) * nids);

Per my hints in my RFC patch, we should really be calling vshMalloc(ctl,
sizeof(int)*(nids + 1)), then checking that the resulting value is
smaller than our allocated size, if we want to be avoiding (some)
aspects of the inherent races.

> +
> +        if ((nids = virConnectListDomains(ctl->conn, ids, nids)) < 0) {
> +            vshError(ctl, "%s", _("Failed to list active domains"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    if ((nnames = virConnectNumOfDefinedDomains(ctl->conn)) < 0) {
> +        vshError(ctl, "%s", _("Failed to list inactive domains"));
> +        goto cleanup;
> +    }

When you add in flags for filtering, you can skip parts of this
back-compat code based on particular flags.

> +
> +    if (nnames) {
> +        names = vshMalloc(ctl, sizeof(char *) * nnames);

Again, using (nnames+1) for the allocation size will at least warn you
if you hit one of the data races.

> +
> +/* collection of filters */
> +static int
> +vshDomainListActiveFilter(vshControl *ctl ATTRIBUTE_UNUSED,
> +                          virDomainPtr dom)
> +{
> +    return virDomainGetID(dom) != (unsigned int) -1;
>  }

I'm not sure if you need these callback functions here, or if you can
fold them into collecting the list in the first place.


> @@ -976,167 +1178,88 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return false;
> 
> -    if (active) {
> -        maxid = virConnectNumOfDomains(ctl->conn);
> -        if (maxid < 0) {
> -            vshError(ctl, "%s", _("Failed to list active domains"));
> -            return false;
> -        }
> -        if (maxid) {
> -            ids = vshMalloc(ctl, sizeof(int) * maxid);
> +    if (vshDomList(ctl, &list) < 0)

I think it would be worth adding the flags into the public API for v2 of
this series, then here you would map 'active' to
VIR_CONNECT_LIST_DOMAINS_ACTIVE, and so forth.

Overall, though, I'm happy with the way this is moving - it is indeed
much easier to operate on a list of virDomainPtr, especially if that
list has been pre-filtered.

-- 
Eric Blake   eblake at 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: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120521/f65e31af/attachment-0001.sig>