On 06/20/2012 07:33 AM, Peter Krempa wrote:
> This patch makes use of the newly added api virConnectListAllDomains()
> to list domains in virsh.
>
> 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, the function vshDomainListCollect was added
s/compatiblity/compatibility/
> 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 by all
> currently supported flags added with virConnectListAllDomains().
>
> This patch also cleans up the "list" command handler to use the new
> helpers and adds new command line flags to make use of filtering.
> ---
> Diff to v3:
> -Fixed compacting of domain list in fallback filter
> -removed redundant filter for active state
> -split out re-indentation and rename of namesorter to a separate patch
> -renamed functions
> -renamed flags
> -tweaked docs (typos/grammar)
> +static void
> +vshDomainListFree(vshDomainListPtr domlist)
> +{
> + int i;
> +
> + if (!domlist || !domlist->domains)
> + return;
Memory leak if domlist && !domlist->domains.
> +
> + if (last_error && last_error->code == VIR_ERR_INVALID_ARG) {
> + /* try the new API again but mask non-guaranteed flags */
> + unsigned int newflags = flags & (VIR_CONNECT_LIST_DOMAINS_ACTIVE |
> + VIR_CONNECT_LIST_DOMAINS_INACTIVE);
Might be easier to use VIR_CONNECT_LIST_FILTERS_ACTIVE from virdomainlist.h.
> + /* fall back to old method (0.9.12 and older) */
> + virResetLastError();
> +
> + /* list active domains, if necessary */
> + if (!MATCH(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
> + VIR_CONNECT_LIST_DOMAINS_INACTIVE) ||
Again, using ...LIST_FILTERS_ACTIVE might be easier.
> +
> + /* truncate domanis that weren't found */
s/domanis/domains/
> + deleted = (nids + nnames) - list->ndomains;
> +
> +filter:
> + /* filter list the list if the list was acquired by fallback means */
> + for (i = 0; i < list->ndomains; i++) {
> + dom = list->domains[i];
> +
> + /* persistence filter */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
> + VIR_CONNECT_LIST_DOMAINS_TRANSIENT)) {
VIR_CONNECT_LIST_FILTERS_PERSISTENT (and so forth, I'll quit pointing it
out)
> + /* autostart filter */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
> + VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART)) {
> + if (virDomainGetAutostart(dom, &autostart) < 0) {
> + vshError(ctl, "%s", _("Failed to get domain autostart state"));
> + goto cleanup;
> + }
Here, I think it is safe to assume that failure of
virDomainGetAutostart() implies that we can treat the domain like it
does not support autostart (that is, match DOMAINS_NO_AUTOSTART),
instead of erroring out. But up to you; I'm also okay with the error,
because in that case, you can avoid the error by not passing in the flag
to virsh in the first place.
> +
> + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART) && autostart) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART) && !autostart)))
> + goto remove_entry;
> + }
> +
> + /* managed save filter */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
> + VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE)) {
> + if ((mansave = virDomainHasManagedSaveImage(dom, 0)) < 0) {
> + vshError(ctl, "%s",
> + _("Failed to check for managed save image"));
> + goto cleanup;
> + }
Same story - if the API fails, you can probably safely assume there is
no managed save image.
> +
> + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE) && mansave) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE) && !mansave)))
> + goto remove_entry;
> + }
> +
> + /* snapshot filter */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
> + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT)) {
> + if ((nsnap = virDomainSnapshotNum(dom, 0)) < 0) {
> + vshError(ctl, "%s", _("Failed to get snapshot count"));
> + goto cleanup;
> + }
Likewise, if the API fails, you can assume no snapshots.
> +
> +To filter the list of domains present on the hypervisor you may specify one or
> +more of filtering flags supported by the B<list> command. These flags are
s/more of/more/
ACK with typo fixes and memory leak plugged. I'm okay if you push
without posting a v5, now that the fixes are down to a manageable amount.
--
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