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

Re: [libvirt] [PATCH 03/49] list: Define new API virStorageListALlStoragePools



On 07/20/2012 08:24 AM, Osier Yang wrote:
> This introduces a new API to list the storage pool objects,
> 4 groups of flags are provided to filter the returned pools:
> 
>   * Active or not
> 
>   * Autostarting or not
> 
>   * Persistent or not
> 
>   * And the pool type.
> 
> include/libvirt/libvirt.h.in: New enum virConnectListAllStoragePoolFlags;
>                               Declare the API.
> python/generator.py: Skip the generating
> src/driver.h: (virDrvConnectListAllStoragePools)
> src/libvirt.c: Implementation for the API.
> src/libvirt_public.syms: Export the symbol.

> +++ b/include/libvirt/libvirt.h.in
> @@ -2471,6 +2471,38 @@ int                     virConnectListDefinedStoragePools(virConnectPtr conn,
>                                                            int maxnames);
>  
>  /*
> + * virConnectListAllStoragePoolsFlags:
> + *
> + * Flags used to filter the returned pools. Flags in each group
> + * are exclusive attribute for a pool.

In virConnectListAllDomainsFlags, we used a different wording:

Flags used to tune pools returned by virConnectListAllStoragePools().
Note that these flags come in groups; if all bits from a group are 0,
then that group is not used to filter results.

> +++ b/src/libvirt.c
> @@ -11215,6 +11215,83 @@ virStoragePoolGetConnect (virStoragePoolPtr pool)
>  }
>  
>  /**
> + * virConnectListAllStoragePools:
> + * @conn: Pointer to the hypervisor connection.
> + * @pools: Pointer to a variable to store the array containing storage pool
> + *         objects or NULL if the list is not required (just returns number
> + *         of pools).
> + * @flags: bitwise-OR of virConnectListAllStoragePoolsFlags.
> + *
> + * Collect the list of storage pools, and allocate an array to store those
> + * objects. This API solves the race inherent between virConnectListStoragePools
> + * and virConnectListDefinedStoragePools.

It would also be worth enhancing the documentation of those two
functions to point out the race, and cross-reference to this function.
For a prior example, see how commit 747f64ee did things.

> + *
> + * By default, this API covers all storage pools; it is also possible
> + * to return the filtered objects with flags. Filters are provided in groups,
> + * where each group contains bits that describe mutually exclusive attributes
> + * of a storage pool.

The wording in the prior example was:

Normally, all storage pools are returned; however, @flags can be used to
filter the results for a smaller list of targeted pools.  The valid
flags are divided into groups, where each group contains bits that
describe mutually exclusive attributes of a pool, and where all bits
within a group describe all possible pools.

> + *
> + * The first group of @flags is VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE and
> + * VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE to fitler the pools by state.

s/fitler/filter/


> @@ -11256,9 +11333,11 @@ error:
>   * @names: array of char * to fill with pool names (allocated by caller)
>   * @maxnames: size of the names array
>   *
> - * Provides the list of names of active storage pools
> - * upto maxnames. If there are more than maxnames, the
> - * remaining names will be silently ignored.
> + * Provides the list of names of active storage pools upto maxnames.

s/upto/up to/

> + * If there are more than maxnames, the remaining names will be silently
> + * ignored.
> + *
> + * For more control over the results, see virConnectListAllStoragePools().
>   *
>   * Returns 0 on success, -1 on error

This lists the cross-reference, but not the fact that there is an
inherent race.  Based on the prior example, this should be something like:

Returns the number of pools found or -1 in case of error.  Note that
this command is inherently racy; a pool can be started between a call to
virConnectNumOfStoragePools() and this call; you are only guaranteed
that all currently active pools were listed if the return is less than
@maxnames.

> +++ b/src/libvirt_public.syms
> @@ -544,4 +544,9 @@ LIBVIRT_0.9.13 {
>          virDomainSnapshotRef;
>  } LIBVIRT_0.9.11;
>  
> +LIBVIRT_0.9.14 {
> +    global:
> +        virConnectListAllStoragePools;
> +} LIBVIRT_0.9.13;

Merge conflict with Guido's patch a91067f.  Should be easy to resolve.

-- 
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]