[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4 2/6] virsh: add support for virConnectListAllDomains and clean up cmdList



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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org



Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]