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@xxxxxxxxxx +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