On 06/05/2012 07:19 AM, Peter Krempa wrote: > This patch makes use of the newly added api virConnectListAllDomains() > to list domains in virsh. I would rebase this patch to come right after 1/9 but before any driver implementation. Why? Because then you will have automatic testing that the fallback code actually works, prior to implementing the new API in later patches. In fact, I'm reviewing it out of order (before 5/9) so that I can test the fallback mode first. > > To enable fallback virsh now represents lists of domains using an > internal structure vshDomainList. This structure contains the > virDomainPtr list as provided by virConnectListAllDomains() and the > count of domains in the list. > > For backwards compatiblity function vshDomList was added that tries to > enumerate the domains using the new API and if the API is not supported > falls back to the older approach with the two list functions. The helper > function also simulates filtering of some of the vlags supported by > virConnectListAllDomains(). > > This patch also cleans up the "list" command handler to use the new > helpers. > --- > Diff to v1: > - moved filtering to the helper function > - cleaned up flag handling in cmdList > --- > tools/virsh.c | 469 ++++++++++++++++++++++++++++++++++++--------------------- > 1 files changed, 297 insertions(+), 172 deletions(-) Question - should we be adding new flags to virsh.c for 'virsh list', and therefore new documentation to virsh.pod, to expose the new filtering bits? But that would be okay as a followup patch. I've got a pending patch series in my sandbox for virDomainListAllSnapshots, where I split things into adding new filter arguments in virsh prior to adding the new list API (hmm, I'd better dust that work off and get it posted now, but it is based on top of this as-yet-unreviewed series, if you want to trade reviews :) https://www.redhat.com/archives/libvir-list/2012-May/msg01196.html I don't know what happened, but this doesn't compile (probably a mistake on your part in a last-minute silencing of a 'make syntax-check' failure): virsh.c: In function 'cmdList': virsh.c:1285:13: error: too few arguments to function 'virStrncpy' In file included from virsh.c:53:0: > +static int > +vshDomList(vshControl *ctl, vshDomainListPtr domlist, unsigned int flags) > +{ > + int rv = -1; > + virErrorPtr err = NULL; > + int i; > + int *ids = NULL; > + int nids = 0; > + char **names = NULL; > + int nnames = 0; > + virDomainPtr dom; > + virDomainPtr *doms = NULL; > + int ndoms = 0; > + > + domlist->domains = NULL; > + domlist->ndomains = 0; > + > + /* try the list with flags support */ > + if ((ndoms = virConnectListAllDomains(ctl->conn, &doms, flags)) >= 0) { > + domlist->ndomains = ndoms; > + domlist->domains = doms; > + doms = NULL; > + goto finished; > + } So much easier when we are done here :) > + > + /* check if the command is actualy supported or it was an error */ s/actualy/actually/ > + if ((err = virGetLastError()) && > + !(err->code == VIR_ERR_NO_SUPPORT || > + err->code == VIR_ERR_INVALID_ARG)) { > + vshError(ctl, "%s", _("Failed to list domains")); > + goto cleanup; > + } Careful. VIR_ERR_NO_SUPPORT means that virConnectListAllDomains() was not supported, so we have to use the old method to construct the list. But VIR_ERR_INVALID_ARG means that virConnectListAllDomains() _is_ supported, just that it doesn't support the full range of flags according to what we passed in. In that case, what we should do is call virConnectListAllDomains(ctl->conn, &doms, 0), then we can 'goto filters' where we use the same code to run filters as the old method (at least we grabbed the list race-free). [1] Hmm, I think we need to guarantee in the API documentation in 1/9 that all drivers are guaranteed to support _ACTIVE, _INACTIVE, _PERSISTENT, _TRANSIENT, and the four state flags; and only permit failures on the remaining 3 groups (autostart, snapshot, managedsave), since only those latter features are optional for a given driver (and since we might add future optional groups). Then it would be a case of: virConnectListAllDomains(ctl->conn, &doms, flags & (_ACTIVE|_INACTIVE)) rather than 0, before jumping to the filter label. > + > + /* get active domains */ > + for (i = 0; i < nids; i++) { > + if (!(dom = virDomainLookupByID(ctl->conn, ids[i]))) > + continue; > + doms[ndoms++] = dom; > + } > + > + /* get inctive domains */ s/inctive/inactive/ > + for (i = 0; i < nnames; i++) { > + if (!(dom = virDomainLookupByName(ctl->conn, names[i]))) > + continue; > + doms[ndoms++] = dom; > + } > + > + /* filter list the list if the list was acquired by fallback means */ [1] Need the 'filter' label here. > + domlist->domains = doms; > + domlist->ndomains = ndoms; > + doms = NULL; > + > + /* filter by active state */ > + /* already done while getting the list by fallback means */ > + > + /* filter by persistent state */ > + if (!(flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT && > + flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) { > + /* remove transient domains */ > + if (flags & VIR_CONNECT_LIST_DOMAINS_PERSISTENT && > + vshDomListFilter(ctl, domlist, vshDomListPersistFilter, false) < 0) > + goto cleanup; > + > + /* remove persistent domains */ > + if (flags & VIR_CONNECT_LIST_DOMAINS_TRANSIENT && > + vshDomListFilter(ctl, domlist, vshDomListPersistFilter, true) < 0) > + goto cleanup; > + } > + > + /* reject all other filters - they are not needed by now */ > + if (flags & > + ~(VIR_CONNECT_LIST_DOMAINS_ACTIVE | > + VIR_CONNECT_LIST_DOMAINS_INACTIVE | > + VIR_CONNECT_LIST_DOMAINS_PERSISTENT | > + VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) { > + vshError(ctl, "%s", _("Filter flags unsupported in fallback filter")); > + goto cleanup; > + } The goal of virsh is to expose all the flags of the underlying API, so we should eventually be supporting all the flags, even if we have to error out when we get to the fallback paths. > + for (i = 0; i < list.ndomains; i++) { > + if (virDomainGetID(list.domains[i]) != (unsigned int) -1) > + snprintf(id, sizeof(id), "%d", virDomainGetID(list.domains[i])); > + else > + virStrncpy(id, "-", sizeof(id)); This was the compile failure. Once I fixed it to virStrcpyStatic(id, "-"), my smoke testing of the fallback mode appeared to do the right things. Overall, I like the way this is headed. The diffstat lies - we may have added more lines than we removed, but the end result is that vshCmdList is much simpler to read and we now have a reusable listing mechanism for anything else in this file (or even as an example for other applications to borrow from in using the same fallback methods). I do think a v3 would be worthwhile, though, especially if you add new flags to 'virsh list'. -- 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