[libvirt] RFE: virConnectListAllDomains()

[Daniel P. Berrange]

On Fri, May 18, 2012 at 05:52:36PM -0600, Eric Blake wrote:
> Use of virConnectListDomains() and virConnectListDefinedDomains() is:
> 
> 1. inherently racy.  A domain can change between active and inactive
> between two back-to-back calls, and thus be entirely skipped or
> enumerated twice when concatenating lists.
> 
> 2. painful to use.  ListDomains gives ids, ListDefinedDomains gives
> names, and the user must then call virDomainLookupByID() and
> virDomainLookupByName() to convert into UUIDs.
> 
> 3. requires pre-allocation.  The user must call virConnectNumOfDomains()
> then over-allocate before calling virConnectListDomains(), in order to
> guarantee that the list size didn't change between the two calls.

Likewise virConnectList{StoragePools,Secrets,Networks,Interfaces,NodeDevices}
and virStoragePoolListVolumes

> 
> This is a proposal for a new API that addresses all three points - by
> returning virDomainPtr rather than id or strings, the UUID of each
> domain can be grabbed in one shot.  By consolidating things into a
> single API call, there is no race in trying to piece together the
> complete list.  By having libvirt allocate the resulting array, rather
> than making the caller pre-allocate, the user doesn't have to worry
> about a race between getting a count and using that count.  It also
> provides the convenience of returning smaller lists based on various
> filtering groups.
> 
> Thoughts before I expand this API and add the actual implementation?

This was one of the items on my "TODO" list that I considered
neccessary before a 1.0 release of libvirt. IF we do this, we
should of course also do the same for the other similar APIs
we have with race conditions.

> +/**
> + * virConnectListAllDomainsFlags:
> + *
> + * Flags used to tune which domains are listed by
> virConnectListAllDomains().
> + * Note that these flags come in groups; if all bits from a group are 0,
> + * then that group is not used to filter results.

Hmm, interesting idea.

> + */
> +typedef enum {
> +    VIR_CONNECT_LIST_DOMAINS_ACTIVE         = 1 << 0,
> +    VIR_CONNECT_LIST_DOMAINS_INACTIVE       = 1 << 1,
> +
> +    VIR_CONNECT_LIST_DOMAINS_PERSISTENT     = 1 << 2,
> +    VIR_CONNECT_LIST_DOMAINS_TRANSIENT      = 1 << 3,
> +
> +    VIR_CONNECT_LIST_DOMAINS_RUNNING        = 1 << 4,
> +    VIR_CONNECT_LIST_DOMAINS_PAUSED         = 1 << 5,
> +    VIR_CONNECT_LIST_DOMAINS_SHUTOFF        = 1 << 6,
> +    VIR_CONNECT_LIST_DOMAINS_OTHER          = 1 << 7,
> +
> +    VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE    = 1 << 8,
> +    VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE = 1 << 9,
> +
> +    VIR_CONNECT_LIST_DOMAINS_AUTOSTART      = 1 << 10,
> +    VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART   = 1 << 11,
> +
> +    VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT   = 1 << 12,
> +    VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT    = 1 << 13,
> +} virConnectListAllDomainsFlags;
> +
> +int                     virConnectListAllDomains(virConnectPtr conn,
> +                                                 virDomainPtr *doms,
> +                                                 unsigned int flags);
> 
>  /*
>   * Get connection from domain.
> diff --git i/src/libvirt.c w/src/libvirt.c
> index 22fc863..9d7d96f 100644
> --- i/src/libvirt.c
> +++ w/src/libvirt.c
> @@ -1859,7 +1859,14 @@ error:
>   *
>   * Collect the list of active domains, and store their IDs in array @ids
>   *
> - * Returns the number of domains found or -1 in case of error
> + * For inactive domains, see virConnectListDefinedDomains().  For more
> + * control over the results, see virConnectListAllDomains().
> + *
> + * Returns the number of domains found or -1 in case of error.  Note that
> + * this command is inherently racy; a domain can be started between a
> + * call to virConnectNumOfDomains() and this call; you are only guaranteed
> + * that all currently active domains were listed if the return is less
> + * than @maxids.
>   */
>  int
>  virConnectListDomains(virConnectPtr conn, int *ids, int maxids)
> @@ -1927,6 +1934,88 @@ error:
>  }
> 
>  /**
> + * virConnectListAllDomains:
> + * @conn: pointer to the hypervisor connection
> + * @doms: pointer to the location to store allocated output array
> + * @flags: bitwise-OR of virConnectListAllDomainsFlags
> + *
> + * Collect a possibly-filtered list of all domains, and return an allocated
> + * array of information for each.  This API solves the race inherent in
> + * virConnectListDomains() and virConnectListDefinedDomains().
> + *
> + * Normally, all domains are returned; however, @flags can be used to
> + * filter the results for a smaller list of targetted domains.  The valid
> + * flags are divided into groups, where each group contains bits that
> + * describe mutually exclusive attributes of a domain, and where all bits
> + * within a group describe all possible domains.  Some hypervisors might
> + * reject explicit bits from a group where the hypervisor cannot make a
> + * distinction (for example, not all hypervisors can tell whether domains
> + * have snapshots).  For a group supported by a given hypervisor, the
> + * behavior when no bits of a group are set is identical to the behavior
> + * when all bits in that group are set.  When setting bits from more than
> + * one group, it is possible to select an impossible combination (such
> + * as an inactive transient domain), in that case a hypervisor may return
> + * either 0 or an error.
> + *
> + * The first group of @flags is VIR_CONNECT_LIST_DOMAINS_ACTIVE (online
> + * domains) and VIR_CONNECT_LIST_DOMAINS_INACTIVE (offline domains).
> + *
> + * The next group of @flags is VIR_CONNECT_LIST_DOMAINS_PERSISTENT (defined
> + * domains) and VIR_CONNECT_LIST_DOMAINS_TRANSIENT (running but not
> defined).
> + *
> + * The next group of @flags covers various domain states:
> + * VIR_CONNECT_LIST_DOMAINS_RUNNING, VIR_CONNECT_LIST_DOMAINS_PAUSED,
> + * VIR_CONNECT_LIST_DOMAINS_SHUTOFF, and a catch-all for all other states
> + * (such as crashed, this catch-all covers the possibility of adding new
> + * states).
> + *
> + * The remaining groups cover boolean attributes commonly asked about
> + * domains; they include VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE and
> + * VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE, for filtering based on whether
> + * a managed save image exists; VIR_CONNECT_LIST_DOMAINS_AUTOSTART and
> + * VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART, for filtering based on autostart;
> + * VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT and
> + * VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT, for filtering based on whether
> + * a domain has snapshots.
> + *
> + * Returns the number of domains found or -1 in case of error.  On success,
> + * the array stored into @doms is guaranteed to have an extra allocated
> + * element set to NULL, to make iteration easier.  The caller is
> + * responsible for calling virDomainFree() on each array element, then
> + * calling free() on @doms.
> + */
> +int
> +virConnectListDomains(virConnectPtr conn, int *ids, int maxids)

The signature is wrong here, but I know what you intended

> +{
> +    VIR_DEBUG("conn=%p, ids=%p, maxids=%d", conn, ids, maxids);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    if ((ids == NULL) || (maxids < 0)) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    if (conn->driver->listDomains) {
> +        int ret = conn->driver->listDomains (conn, ids, maxids);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +error:
> +    virDispatchError(conn);
> +    return -1;
> +}
> +
> +/**
>   * virDomainGetConnect:
>   * @dom: pointer to a domain
>   *
> @@ -8092,7 +8181,14 @@ error:
>   * list the defined but inactive domains, stores the pointers to the names
>   * in @names
>   *
> - * Returns the number of names provided in the array or -1 in case of error
> + * For active domains, see virConnectListDomains().  For more control over
> + * the results, see virConnectListAllDomains().
> + *
> + * Returns the number of names provided in the array or -1 in case of
> error.
> + * Note that this command is inherently racy; a domain can be defined
> between
> + * a call to virConnectNumOfDefinedDomains() and this call; you are only
> + * guaranteed that all currently defined domains were listed if the return
> + * is less than @maxids.  The client must call free() on each returned
> name.
>   */
>  int
>  virConnectListDefinedDomains(virConnectPtr conn, char **const names,


This gets my vote

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|