[libvirt] [PATCHv2 9/9] virsh: add support for virConnectListAllDomains and clean up cmdList
Eric Blake
eblake at redhat.com
Tue Jun 5 23:07:20 UTC 2012
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 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/20120605/f5023fc2/attachment-0001.sig>
More information about the libvir-list
mailing list