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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list