[libvirt] [PATCH v4 00/10] Atomic API to list storage pools

Think the subject tells enough after 3 rounds, so ommits words here. v3 - v4: * Just rebase on the top: - Version changes - rebase after virsh split cleanups 1/10 ~ 2/10 are ACK'ed in v3, but for easy viewing, they are still posted. Osier Yang (10): list: Define new API virStorageListAllStoragePools list: Add helpers for listing storage pool objects list: Implement the RPC calls for virConnectListAllStoragePools list: Implement listAllStoragePools for storage driver list: Implement listAllStoragePools for test driver list: Add helper to convert strings separated by ', ' to array virsh: Fix the wrong doc for pool-list list: Change MATCH for common use in virsh list: Use virConnectListAllStoragePools in virsh python: Expose virStorageListAllStoragePools to python binding daemon/remote.c | 54 +++ include/libvirt/libvirt.h.in | 33 ++ python/generator.py | 5 +- python/libvirt-override-api.xml | 6 + python/libvirt-override-virConnect.py | 12 + python/libvirt-override.c | 47 +++ src/conf/storage_conf.c | 116 ++++++ src/conf/storage_conf.h | 35 ++ src/driver.h | 5 + src/libvirt.c | 112 ++++++- src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 + src/remote/remote_driver.c | 64 ++++ src/remote/remote_protocol.x | 13 +- src/remote_protocol-structs | 12 + src/storage/storage_driver.c | 18 + src/test/test_driver.c | 17 + tools/virsh-domain-monitor.c | 2 - tools/virsh-domain.c | 19 +- tools/virsh-pool.c | 631 ++++++++++++++++++++++++++++++++- tools/virsh.c | 44 +++ tools/virsh.h | 3 + tools/virsh.pod | 33 ++- 23 files changed, 1249 insertions(+), 38 deletions(-) -- 1.7.7.3

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. --- include/libvirt/libvirt.h.in | 33 ++++++++++++ python/generator.py | 5 +- src/driver.h | 5 ++ src/libvirt.c | 112 +++++++++++++++++++++++++++++++++++++++--- src/libvirt_public.syms | 5 ++ 5 files changed, 150 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index cfe5047..0c52271 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2554,6 +2554,39 @@ int virConnectListDefinedStoragePools(virConnectPtr conn, int maxnames); /* + * virConnectListAllStoragePoolsFlags: + * + * 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. + */ +typedef enum { + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE = 1 << 0, + VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE = 1 << 1, + + VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT = 1 << 2, + VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT = 1 << 3, + + VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART = 1 << 4, + VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART = 1 << 5, + + /* List pools by type */ + VIR_CONNECT_LIST_STORAGE_POOLS_DIR = 1 << 6, + VIR_CONNECT_LIST_STORAGE_POOLS_FS = 1 << 7, + VIR_CONNECT_LIST_STORAGE_POOLS_NETFS = 1 << 8, + VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL = 1 << 9, + VIR_CONNECT_LIST_STORAGE_POOLS_DISK = 1 << 10, + VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI = 1 << 11, + VIR_CONNECT_LIST_STORAGE_POOLS_SCSI = 1 << 12, + VIR_CONNECT_LIST_STORAGE_POOLS_MPATH = 1 << 13, + VIR_CONNECT_LIST_STORAGE_POOLS_RBD = 1 << 14, + VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG = 1 << 15, +} virConnectListAllStoragePoolsFlags; + +int virConnectListAllStoragePools(virConnectPtr conn, + virStoragePoolPtr **pools, + unsigned int flags); +/* * Query a host for storage pools of a particular type */ char * virConnectFindStoragePoolSources(virConnectPtr conn, diff --git a/python/generator.py b/python/generator.py index 7beb361..261efe0 100755 --- a/python/generator.py +++ b/python/generator.py @@ -337,7 +337,7 @@ foreign_encoding_args = ( # ####################################################################### -# Class methods which are written by hand in libvir.c but the Python-level +# Class methods which are written by hand in libvirt.c but the Python-level # code is still automatically generated (so they are not in skip_function()). skip_impl = ( 'virConnectGetVersion', @@ -457,9 +457,10 @@ skip_function = ( 'virConnectDomainEventDeregisterAny', # overridden in virConnect.py 'virSaveLastError', # We have our own python error wrapper 'virFreeError', # Only needed if we use virSaveLastError - 'virConnectListAllDomains', #overridden in virConnect.py + 'virConnectListAllDomains', # overridden in virConnect.py 'virDomainListAllSnapshots', # overridden in virDomain.py 'virDomainSnapshotListAllChildren', # overridden in virDomainSnapshot.py + 'virConnectListAllStoragePools', # overridden in virConnect.py 'virStreamRecvAll', # Pure python libvirt-override-virStream.py 'virStreamSendAll', # Pure python libvirt-override-virStream.py diff --git a/src/driver.h b/src/driver.h index e88ab28..342f6b5 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1252,6 +1252,10 @@ typedef int (*virDrvConnectListDefinedStoragePools) (virConnectPtr conn, char **const names, int maxnames); +typedef int + (*virDrvConnectListAllStoragePools) (virConnectPtr conn, + virStoragePoolPtr **pools, + unsigned int flags); typedef char * (*virDrvConnectFindStoragePoolSources) (virConnectPtr conn, const char *type, @@ -1396,6 +1400,7 @@ struct _virStorageDriver { virDrvConnectListStoragePools listPools; virDrvConnectNumOfDefinedStoragePools numOfDefinedPools; virDrvConnectListDefinedStoragePools listDefinedPools; + virDrvConnectListAllStoragePools listAllPools; virDrvConnectFindStoragePoolSources findPoolSources; virDrvStoragePoolLookupByName poolLookupByName; virDrvStoragePoolLookupByUUID poolLookupByUUID; diff --git a/src/libvirt.c b/src/libvirt.c index b034ed6..4e4f96b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -11416,6 +11416,90 @@ 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. + * + * 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_ACTIVE (online) + * and VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE (offline) to filter the pools + * by state. + * + * The second group of @flags is VIR_CONNECT_LIST_STORAGE_POOLS_PERSITENT + * (defined) and VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT (running but not + * defined), to filter the pools by whether they have persistent config or not. + * + * The third group of @flags is VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART + * and VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART, to filter the pools by + * whether they are marked as autostart or not. + * + * The last group of @flags is provided to filter the pools by the types, + * the flags include: + * VIR_CONNECT_LIST_STORAGE_POOLS_DIR + * VIR_CONNECT_LIST_STORAGE_POOLS_FS + * VIR_CONNECT_LIST_STORAGE_POOLS_NETFS + * VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL + * VIR_CONNECT_LIST_STORAGE_POOLS_DISK + * VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI + * VIR_CONNECT_LIST_STORAGE_POOLS_SCSI + * VIR_CONNECT_LIST_STORAGE_POOLS_MPATH + * VIR_CONNECT_LIST_STORAGE_POOLS_RBD + * VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG + * + * Returns the number of storage pools found or -1 and sets @pools to + * NULL in case of error. On success, the array stored into @pools is + * guaranteed to have an extra allocated element set to NULL but not included + * in the return count, to make iteration easier. The caller is responsible + * for calling virStoragePoolFree() on each array element, then calling + * free() on @pools. + */ +int +virConnectListAllStoragePools(virConnectPtr conn, + virStoragePoolPtr **pools, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, pools=%p, flags=%x", conn, pools, flags); + + virResetLastError(); + + if (pools) + *pools = NULL; + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (conn->storageDriver && + conn->storageDriver->listAllPools) { + int ret; + ret = conn->storageDriver->listAllPools(conn, pools, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** * virConnectNumOfStoragePools: * @conn: pointer to hypervisor connection * @@ -11457,11 +11541,17 @@ 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 up to maxnames. + * If there are more than maxnames, the remaining names will be silently + * ignored. * - * Returns 0 on success, -1 on error + * For more control over the results, see virConnectListAllStoragePools(). + * + * 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. */ int virConnectListStoragePools(virConnectPtr conn, @@ -11540,11 +11630,17 @@ 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 inactive storage pools - * upto maxnames. If there are more than maxnames, the - * remaining names will be silently ignored. + * Provides the list of names of inactive storage pools up to maxnames. + * If there are more than maxnames, the remaining names will be silently + * ignored. * - * Returns 0 on success, -1 on error + * For more control over the results, see virConnectListAllStoragePools(). + * + * Returns the number of names provided in the array or -1 in case of error. + * Note that this command is inherently racy; a pool can be defined between + * a call to virConnectNumOfDefinedStoragePools() and this call; you are only + * guaranteed that all currently defined pools were listed if the return + * is less than @maxnames. The client must call free() on each returned name. */ int virConnectListDefinedStoragePools(virConnectPtr conn, diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 92ae95a..f6068b4 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -554,4 +554,9 @@ LIBVIRT_0.10.0 { virDomainGetEmulatorPinInfo; } LIBVIRT_0.9.13; +LIBVIRT_0.10.2 { + global: + virConnectListAllStoragePools; +} LIBVIRT_0.10.0; + # .... define new API here using predicted next version number .... -- 1.7.7.3

On 09/04/2012 09:16 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.
+++ b/src/libvirt_public.syms @@ -554,4 +554,9 @@ LIBVIRT_0.10.0 { virDomainGetEmulatorPinInfo; } LIBVIRT_0.9.13;
+LIBVIRT_0.10.2 { + global: + virConnectListAllStoragePools; +} LIBVIRT_0.10.0;
Good, you got this right on your rebase. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

src/conf/storage_conf.c: Add virStoragePoolMatch to filter the pools; Add virStoragePoolList to iterate over the pool objects with filter. src/conf/storage_conf.h: Declare virStoragePoolMatch, virStoragePoolList, and the macros for filters. src/libvirt_private.syms: Export helper virStoragePoolList. --- src/conf/storage_conf.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 35 ++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 152 insertions(+), 0 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3132aae..b14564f 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1883,3 +1883,119 @@ void virStoragePoolObjUnlock(virStoragePoolObjPtr obj) { virMutexUnlock(&obj->lock); } + +#define MATCH(FLAG) (flags & (FLAG)) +static bool +virStoragePoolMatch(virStoragePoolObjPtr poolobj, + unsigned int flags) +{ + /* filter by active state */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) && + !((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE) && + virStoragePoolObjIsActive(poolobj)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE) && + !virStoragePoolObjIsActive(poolobj)))) + return false; + + /* filter by persistence */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_PERSISTENT) && + !((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT) && + poolobj->configFile) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT) && + !poolobj->configFile))) + return false; + + /* filter by autostart option */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART) && + !((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART) && + poolobj->autostart) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART) && + !poolobj->autostart))) + return false; + + /* filter by pool type */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE)) { + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_DIR) && + (poolobj->def->type == VIR_STORAGE_POOL_DIR)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FS) && + (poolobj->def->type == VIR_STORAGE_POOL_FS)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_NETFS) && + (poolobj->def->type == VIR_STORAGE_POOL_NETFS)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL) && + (poolobj->def->type == VIR_STORAGE_POOL_LOGICAL)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_DISK) && + (poolobj->def->type == VIR_STORAGE_POOL_DISK)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI) && + (poolobj->def->type == VIR_STORAGE_POOL_ISCSI)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_SCSI) && + (poolobj->def->type == VIR_STORAGE_POOL_SCSI)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_MPATH) && + (poolobj->def->type == VIR_STORAGE_POOL_MPATH)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_RBD) && + (poolobj->def->type == VIR_STORAGE_POOL_RBD)) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG) && + (poolobj->def->type == VIR_STORAGE_POOL_SHEEPDOG)))) + return false; + } + + return true; +} +#undef MATCH + +int +virStoragePoolList(virConnectPtr conn, + virStoragePoolObjList poolobjs, + virStoragePoolPtr **pools, + unsigned int flags) +{ + virStoragePoolPtr *tmp_pools = NULL; + virStoragePoolPtr pool = NULL; + int npools = 0; + int ret = -1; + int i; + + if (pools) { + if (VIR_ALLOC_N(tmp_pools, poolobjs.count + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + } + + for (i = 0; i < poolobjs.count; i++) { + virStoragePoolObjPtr poolobj = poolobjs.objs[i]; + virStoragePoolObjLock(poolobj); + if (virStoragePoolMatch(poolobj, flags)) { + if (pools) { + if (!(pool = virGetStoragePool(conn, + poolobj->def->name, + poolobj->def->uuid))) { + virStoragePoolObjUnlock(poolobj); + goto cleanup; + } + tmp_pools[npools] = pool; + } + npools++; + } + virStoragePoolObjUnlock(poolobj); + } + + if (tmp_pools) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(tmp_pools, npools + 1)); + *pools = tmp_pools; + tmp_pools = NULL; + } + + ret = npools; + +cleanup: + if (tmp_pools) { + for (i = 0; i < npools; i++) { + if (tmp_pools[i]) + virStoragePoolFree(tmp_pools[i]); + } + } + + VIR_FREE(tmp_pools); + return ret; +} diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 4fb99df..bfa0819 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -518,4 +518,39 @@ enum virStoragePartedFsType { }; VIR_ENUM_DECL(virStoragePartedFsType) +# define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE \ + (VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | \ + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE) + +# define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_PERSISTENT \ + (VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT | \ + VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT) + +# define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART \ + (VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART | \ + VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART) + +# define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE \ + (VIR_CONNECT_LIST_STORAGE_POOLS_DIR | \ + VIR_CONNECT_LIST_STORAGE_POOLS_FS | \ + VIR_CONNECT_LIST_STORAGE_POOLS_NETFS | \ + VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL | \ + VIR_CONNECT_LIST_STORAGE_POOLS_DISK | \ + VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI | \ + VIR_CONNECT_LIST_STORAGE_POOLS_SCSI | \ + VIR_CONNECT_LIST_STORAGE_POOLS_MPATH | \ + VIR_CONNECT_LIST_STORAGE_POOLS_RBD | \ + VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG) + +# define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL \ + (VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE | \ + VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_PERSISTENT | \ + VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART | \ + VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE) + +int virStoragePoolList(virConnectPtr conn, + virStoragePoolObjList poolobjs, + virStoragePoolPtr **pools, + unsigned int flags); + #endif /* __VIR_STORAGE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65067d6..abfee47 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1058,6 +1058,7 @@ virStoragePoolDefParseString; virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; +virStoragePoolList; virStoragePoolLoadAllConfigs; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
src/conf/storage_conf.c: Add virStoragePoolMatch to filter the pools; Add virStoragePoolList to iterate over the pool objects with filter.
src/conf/storage_conf.h: Declare virStoragePoolMatch, virStoragePoolList, and the macros for filters.
src/libvirt_private.syms: Export helper virStoragePoolList. --- src/conf/storage_conf.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 35 ++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 152 insertions(+), 0 deletions(-)
As mentioned in 0/10, my ACK still stands. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The RPC generator doesn't support returning list of object, this patch do the work manually. * daemon/remote.c: Implement the server side handler remoteDispatchConnectListAllStoragePools * src/remote/remote_driver.c: Add remote driver handler remoteConnectListAllStoragePools. * src/remote/remote_protocol.x: New RPC procedure REMOTE_PROC_CONNECT_LIST_ALL_STORAGE_POOLS and structs to represent the args and ret for it. * src/remote_protocol-structs: Likewise. --- daemon/remote.c | 54 +++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 13 ++++++++- src/remote_protocol-structs | 12 ++++++++ 4 files changed, 142 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 24928f4..9056439 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4099,6 +4099,60 @@ cleanup: return rv; } +static int +remoteDispatchConnectListAllStoragePools(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_list_all_storage_pools_args *args, + remote_connect_list_all_storage_pools_ret *ret) +{ + virStoragePoolPtr *pools = NULL; + int npools = 0; + int i; + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if ((npools = virConnectListAllStoragePools(priv->conn, + args->need_results ? &pools : NULL, + args->flags)) < 0) + goto cleanup; + + if (pools && npools) { + if (VIR_ALLOC_N(ret->pools.pools_val, npools) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret->pools.pools_len = npools; + + for (i = 0; i < npools; i++) + make_nonnull_storage_pool(ret->pools.pools_val + i, pools[i]); + } else { + ret->pools.pools_len = 0; + ret->pools.pools_val = NULL; + } + + ret->ret = npools; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (pools) { + for (i = 0; i < npools; i++) + virStoragePoolFree(pools[i]); + VIR_FREE(pools); + } + return rv; +} + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index cf1f079..c1f9044 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2857,6 +2857,69 @@ done: return rv; } +static int +remoteConnectListAllStoragePools (virConnectPtr conn, + virStoragePoolPtr **pools, + unsigned int flags) +{ + int rv = -1; + int i; + virStoragePoolPtr *tmp_pools = NULL; + remote_connect_list_all_storage_pools_args args; + remote_connect_list_all_storage_pools_ret ret; + + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + args.need_results = !!pools; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + if (call(conn, + priv, + 0, + REMOTE_PROC_CONNECT_LIST_ALL_STORAGE_POOLS, + (xdrproc_t) xdr_remote_connect_list_all_storage_pools_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_list_all_storage_pools_ret, + (char *) &ret) == -1) + goto done; + + if (pools) { + if (VIR_ALLOC_N(tmp_pools, ret.pools.pools_len + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < ret.pools.pools_len; i++) { + tmp_pools[i] = get_nonnull_storage_pool(conn, ret.pools.pools_val[i]); + if (!tmp_pools[i]) { + virReportOOMError(); + goto cleanup; + } + } + *pools = tmp_pools; + tmp_pools = NULL; + } + + rv = ret.ret; + +cleanup: + if (tmp_pools) { + for (i = 0; i < ret.pools.pools_len; i++) + if (tmp_pools[i]) + virStoragePoolFree(tmp_pools[i]); + VIR_FREE(tmp_pools); + } + + xdr_free((xdrproc_t) xdr_remote_connect_list_all_storage_pools_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + /*----------------------------------------------------------------------*/ static virDrvOpenStatus ATTRIBUTE_NONNULL (1) @@ -5612,6 +5675,7 @@ static virStorageDriver storage_driver = { .listPools = remoteListStoragePools, /* 0.4.1 */ .numOfDefinedPools = remoteNumOfDefinedStoragePools, /* 0.4.1 */ .listDefinedPools = remoteListDefinedStoragePools, /* 0.4.1 */ + .listAllPools = remoteConnectListAllStoragePools, /* 0.10.0 */ .findPoolSources = remoteFindStoragePoolSources, /* 0.4.5 */ .poolLookupByName = remoteStoragePoolLookupByName, /* 0.4.1 */ .poolLookupByUUID = remoteStoragePoolLookupByUUID, /* 0.4.1 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 085d5d9..9de3404 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2558,6 +2558,15 @@ struct remote_connect_list_all_domains_ret { unsigned int ret; }; +struct remote_connect_list_all_storage_pools_args { + int need_results; + unsigned int flags; +}; + +struct remote_connect_list_all_storage_pools_ret { + remote_nonnull_storage_pool pools<>; + unsigned int ret; +}; /*----- Protocol. -----*/ @@ -2888,7 +2897,9 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_HOSTNAME = 277, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL_LIST = 278, /* skipgen skipgen priority:high */ REMOTE_PROC_DOMAIN_PIN_EMULATOR = 279, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_GET_EMULATOR_PIN_INFO = 280 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_EMULATOR_PIN_INFO = 280, /* skipgen skipgen */ + + REMOTE_PROC_CONNECT_LIST_ALL_STORAGE_POOLS = 281 /* skipgen skipgen priority:high */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index ed40ccb..560299b 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2011,6 +2011,17 @@ struct remote_connect_list_all_domains_ret { } domains; u_int ret; }; +struct remote_connect_list_all_storage_pools_args { + int need_results; + u_int flags; +}; +struct remote_connect_list_all_storage_pools_ret { + struct { + u_int pools_len; + remote_nonnull_domain * pools_val; + } pools; + u_int ret; +}; enum remote_procedure { REMOTE_PROC_OPEN = 1, REMOTE_PROC_CLOSE = 2, @@ -2292,4 +2303,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL_LIST = 278, REMOTE_PROC_DOMAIN_PIN_EMULATOR = 279, REMOTE_PROC_DOMAIN_GET_EMULATOR_PIN_INFO = 280, + REMOTE_PROC_CONNECT_LIST_ALL_STORAGE_POOLS = 281, }; -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
The RPC generator doesn't support returning list of object, this patch do
s/patch do/patch does/
the work manually.
@@ -5612,6 +5675,7 @@ static virStorageDriver storage_driver = { .listPools = remoteListStoragePools, /* 0.4.1 */ .numOfDefinedPools = remoteNumOfDefinedStoragePools, /* 0.4.1 */ .listDefinedPools = remoteListDefinedStoragePools, /* 0.4.1 */ + .listAllPools = remoteConnectListAllStoragePools, /* 0.10.0 */
0.10.2. ACK with that change. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 09/04/2012 05:48 PM, Eric Blake wrote:
On 09/04/2012 09:16 AM, Osier Yang wrote:
The RPC generator doesn't support returning list of object, this patch do
s/patch do/patch does/
the work manually.
@@ -5612,6 +5675,7 @@ static virStorageDriver storage_driver = { .listPools = remoteListStoragePools, /* 0.4.1 */ .numOfDefinedPools = remoteNumOfDefinedStoragePools, /* 0.4.1 */ .listDefinedPools = remoteListDefinedStoragePools, /* 0.4.1 */ + .listAllPools = remoteConnectListAllStoragePools, /* 0.10.0 */
0.10.2.
ACK with that change.
Correction; one other change is also necessary: --- remote_protocol-structs 2012-09-04 18:06:02.780444947 -0600 +++ remote_protocol-struct-t3 2012-09-04 18:06:40.975481943 -0600 @@ -2018,7 +2018,7 @@ struct remote_connect_list_all_storage_pools_ret { struct { u_int pools_len; - remote_nonnull_domain * pools_val; + remote_nonnull_storage_pool * pools_val; } pools; u_int ret; }; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

src/storage/storage_driver.c: Implement listAllStoragePools. --- src/storage/storage_driver.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3dc66db..4f99eb9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2285,6 +2285,23 @@ cleanup: return ret; } +static int +storageListAllPools(virConnectPtr conn, + virStoragePoolPtr **pools, + unsigned int flags) +{ + virStorageDriverStatePtr driver = conn->storagePrivateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL, -1); + + storageDriverLock(driver); + ret = virStoragePoolList(conn, driver->pools, pools, flags); + storageDriverUnlock(driver); + + return ret; +} + static virStorageDriver storageDriver = { .name = "storage", .open = storageOpen, /* 0.4.0 */ @@ -2293,6 +2310,7 @@ static virStorageDriver storageDriver = { .listPools = storageListPools, /* 0.4.0 */ .numOfDefinedPools = storageNumDefinedPools, /* 0.4.0 */ .listDefinedPools = storageListDefinedPools, /* 0.4.0 */ + .listAllPools = storageListAllPools, /* 0.10.0 */ .findPoolSources = storageFindPoolSources, /* 0.4.0 */ .poolLookupByName = storagePoolLookupByName, /* 0.4.0 */ .poolLookupByUUID = storagePoolLookupByUUID, /* 0.4.0 */ -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
src/storage/storage_driver.c: Implement listAllStoragePools. --- src/storage/storage_driver.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-)
@@ -2293,6 +2310,7 @@ static virStorageDriver storageDriver = { .listPools = storageListPools, /* 0.4.0 */ .numOfDefinedPools = storageNumDefinedPools, /* 0.4.0 */ .listDefinedPools = storageListDefinedPools, /* 0.4.0 */ + .listAllPools = storageListAllPools, /* 0.10.0 */
0.10.2 ACK with that fix. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

src/test/test_driver.c: Implement listAllStoragePools --- src/test/test_driver.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index aa4418a..1504251 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3944,6 +3944,22 @@ no_memory: return -1; } +static int +testStorageListAllPools(virConnectPtr conn, + virStoragePoolPtr **pools, + unsigned int flags) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL, -1); + + testDriverLock(privconn); + ret = virStoragePoolList(conn, privconn->pools, pools, flags); + testDriverUnlock(privconn); + + return ret; +} static int testStoragePoolIsActive(virStoragePoolPtr pool) { @@ -5662,6 +5678,7 @@ static virStorageDriver testStorageDriver = { .listPools = testStorageListPools, /* 0.5.0 */ .numOfDefinedPools = testStorageNumDefinedPools, /* 0.5.0 */ .listDefinedPools = testStorageListDefinedPools, /* 0.5.0 */ + .listAllPools = testStorageListAllPools, /* 0.10.0 */ .findPoolSources = testStorageFindPoolSources, /* 0.5.0 */ .poolLookupByName = testStoragePoolLookupByName, /* 0.5.0 */ .poolLookupByUUID = testStoragePoolLookupByUUID, /* 0.5.0 */ -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
src/test/test_driver.c: Implement listAllStoragePools --- src/test/test_driver.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)
@@ -5662,6 +5678,7 @@ static virStorageDriver testStorageDriver = { .listPools = testStorageListPools, /* 0.5.0 */ .numOfDefinedPools = testStorageNumDefinedPools, /* 0.5.0 */ .listDefinedPools = testStorageListDefinedPools, /* 0.5.0 */ + .listAllPools = testStorageListAllPools, /* 0.10.0 */
0.10.2. ACK with that fix. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

tools/virsh.c: New helper function vshStringToArray. tools/virsh.h: Declare vshStringToArray. tools/virsh-domain.c: use the helper in cmdUndefine. --- tools/virsh-domain.c | 19 ++----------------- tools/virsh.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f0ec742..27d5dc1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2443,23 +2443,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) /* tokenize the string from user and save it's parts into an array */ if (volumes) { - /* count the delimiters */ - volume_tok = volumes; - nvolume_tokens = 1; /* we need at least one member */ - while (*volume_tok) { - if (*(volume_tok++) == ',') - nvolume_tokens++; - } - - volume_tokens = vshCalloc(ctl, nvolume_tokens, sizeof(char *)); - - /* tokenize the input string */ - nvolume_tokens = 0; - volume_tok = volumes; - do { - volume_tokens[nvolume_tokens] = strsep(&volume_tok, ","); - nvolume_tokens++; - } while (volume_tok); + if ((nvolume_tokens = vshStringToArray(volumes, &volume_tokens)) < 0) + goto cleanup; } if ((nvolumes = virXPathNodeSet("./devices/disk", ctxt, diff --git a/tools/virsh.c b/tools/virsh.c index 5cf3237..684f7ca 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -166,6 +166,50 @@ vshPrettyCapacity(unsigned long long val, const char **unit) } } +/* + * Convert the strings separated by ',' into array. The caller + * must free the returned array after use. + * + * Returns the length of the filled array on success, or -1 + * on error. + */ +int +vshStringToArray(char *str, + char ***array) +{ + char *str_tok = NULL; + unsigned int nstr_tokens = 0; + char **arr = NULL; + + /* tokenize the string from user and save it's parts into an array */ + if (str) { + nstr_tokens = 1; + + /* count the delimiters */ + str_tok = str; + while (*str_tok) { + if (*str_tok == ',') + nstr_tokens++; + str_tok++; + } + + if (VIR_ALLOC_N(arr, nstr_tokens) < 0) { + virReportOOMError(); + return -1; + } + + /* tokenize the input string */ + nstr_tokens = 0; + str_tok = str; + do { + arr[nstr_tokens] = strsep(&str_tok, ","); + nstr_tokens++; + } while (str_tok); + } + + *array = arr; + return nstr_tokens; +} virErrorPtr last_error; diff --git a/tools/virsh.h b/tools/virsh.h index 8923f94..2a9c6a2 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -328,6 +328,7 @@ int vshAskReedit(vshControl *ctl, const char *msg); int vshStreamSink(virStreamPtr st, const char *bytes, size_t nbytes, void *opaque); double vshPrettyCapacity(unsigned long long val, const char **unit); +int vshStringToArray(char *str, char ***array); /* Typedefs, function prototypes for job progress reporting. * There are used by some long lingering commands like -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
tools/virsh.c: New helper function vshStringToArray. tools/virsh.h: Declare vshStringToArray. tools/virsh-domain.c: use the helper in cmdUndefine. --- tools/virsh-domain.c | 19 ++----------------- tools/virsh.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 47 insertions(+), 17 deletions(-)
ACK. This is a nice refactor for later use. But down the road, do we want to borrow a leaf from qemu's book, and allow for users to input ',,' for a literal comma that does not separate options? If so, then we should merge in the double comma handling from vshParseSnapshotDiskspec (virsh-snapshot.c) into this function, and have that function also refactored to use this helper. Without something like that, users cannot input a literal comma when listing comma-separated arguments. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年09月06日 00:24, Eric Blake wrote:
On 09/04/2012 09:16 AM, Osier Yang wrote:
tools/virsh.c: New helper function vshStringToArray. tools/virsh.h: Declare vshStringToArray. tools/virsh-domain.c: use the helper in cmdUndefine. --- tools/virsh-domain.c | 19 ++----------------- tools/virsh.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 47 insertions(+), 17 deletions(-)
ACK. This is a nice refactor for later use.
But down the road, do we want to borrow a leaf from qemu's book, and allow for users to input ',,' for a literal comma that does not separate options? If so, then we should merge in the double comma handling from vshParseSnapshotDiskspec (virsh-snapshot.c) into this function, and have that function also refactored to use this helper. Without something like that, users cannot input a literal comma when listing comma-separated arguments.
That will be nice, but seems useless for virsh now, the only two commands which have option accepts multiple values are undefine and pool-list. For domain undefine, the volumes can't have ',' in the name (see the schema), for pool-list, pool types can't have ',' either. Regards, Osier

On 09/06/2012 08:38 AM, Osier Yang wrote:
allow for users to input ',,' for a literal comma that does not separate options? If so, then we should merge in the double comma handling from vshParseSnapshotDiskspec (virsh-snapshot.c) into this function, and have that function also refactored to use this helper. Without something like that, users cannot input a literal comma when listing comma-separated arguments.
That will be nice, but seems useless for virsh now, the only two commands which have option accepts multiple values are undefine and pool-list. For domain undefine, the volumes can't have ',' in the name (see the schema), for pool-list, pool types can't have ',' either.
I just pointed out that 'virsh snapshot-create-as' is a third virsh command that accepts a list of comma-separated arguments, but where one of the arguments is a file name and therefore uses ',,' escaping. So I'm proposing that we _also_ merge vshParseSnapshotDiskspec into this common helper routine, and thus make double ',,' parsing common throughout virsh (even if most callers can't have ',' in a list element in the first place); but it can be a separate patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The storage pool's management doesn't relate with a domain, it probably was an intention, but not achieved yet. And the fact is only active pools are listed by default. --- tools/virsh.pod | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index a26c420..98ed4b0 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2187,11 +2187,10 @@ Returns basic information about the I<pool> object. =item B<pool-list> [I<--inactive> | I<--all>] [I<--details>] -List pool objects known to libvirt. By default, only pools in use by -active domains are listed; I<--inactive> lists just the inactive -pools, and I<--all> lists all pools. The I<--details> option instructs -virsh to additionally display pool persistence and capacity related -information where available. +List pool objects known to libvirt. By default, only active pools +are listed; I<--inactive> lists just the inactive pools, and I<--all> +lists all pools. The I<--details> option instructs virsh to additionally +display pool persistence and capacity related information where available. =item B<pool-name> I<uuid> -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
The storage pool's management doesn't relate with a domain, it probably was an intention, but not achieved yet. And the fact is only active pools are listed by default. --- tools/virsh.pod | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Move definition of MATCH from virsh-domain-monitor.c into virsh.h for further use. --- tools/virsh-domain-monitor.c | 2 -- tools/virsh.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 7e252d2..e599159 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1290,7 +1290,6 @@ vshDomainListFree(vshDomainListPtr domlist) VIR_FREE(domlist); } -#define MATCH(FLAG) (flags & (FLAG)) static vshDomainListPtr vshDomainListCollect(vshControl *ctl, unsigned int flags) { @@ -1509,7 +1508,6 @@ cleanup: VIR_FREE(ids); return list; } -#undef MATCH static const vshCmdOptDef opts_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive domains")}, diff --git a/tools/virsh.h b/tools/virsh.h index 2a9c6a2..2758d37 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -47,6 +47,8 @@ # define GETTIMEOFDAY(T) gettimeofday(T, NULL) +# define MATCH(FLAG) (flags & (FLAG)) + /** * The log configuration */ -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
Move definition of MATCH from virsh-domain-monitor.c into virsh.h for further use. --- tools/virsh-domain-monitor.c | 2 -- tools/virsh.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)
Not quite right.
+++ b/tools/virsh.h @@ -47,6 +47,8 @@
# define GETTIMEOFDAY(T) gettimeofday(T, NULL)
+# define MATCH(FLAG) (flags & (FLAG))
If you're going to make the macro reusable, I think it would be better to put it in the VSH_ namespace, and update all existing callers. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年09月06日 00:30, Eric Blake wrote:
On 09/04/2012 09:16 AM, Osier Yang wrote:
Move definition of MATCH from virsh-domain-monitor.c into virsh.h for further use. --- tools/virsh-domain-monitor.c | 2 -- tools/virsh.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)
Not quite right.
+++ b/tools/virsh.h @@ -47,6 +47,8 @@
# define GETTIMEOFDAY(T) gettimeofday(T, NULL)
+# define MATCH(FLAG) (flags& (FLAG))
If you're going to make the macro reusable, I think it would be better to put it in the VSH_ namespace, and update all existing callers.
Changed to VSH_MATCH when pushing, with all MATCH replaced by VSH_MATCH in virsh-domain-monitor.c.

tools/virsh-pool.c: * vshStoragePoolSorter to sort the pool list by pool name. * struct vshStoragePoolList to present the pool list, pool info is collected by list->poolinfo if 'details' is specified by user. * vshStoragePoolListFree to free the pool list * vshStoragePoolListCollect to collect the pool list, new API virStorageListAllPools is tried first, if it's not supported, fall back to older APIs. * New options --persistent, --transient, --autostart, --no-autostart and --type for pool-list. --persistent or --transient is to filter the returned pool list by whether the pool is persistent or not. --autostart or --no-autostart is to filter the returned pool list by whether the pool is autostarting or not. --type is to filter the pools by pool types. E.g. % virsh pool-list --all --persistent --type dir,disk tools/virsh.pod: * Add documentations for the new options. --- tools/virsh-pool.c | 631 +++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 24 ++- 2 files changed, 652 insertions(+), 3 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index fd239d2..0b328cc 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h" virStoragePoolPtr vshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, @@ -551,6 +552,232 @@ cmdPoolDumpXML(vshControl *ctl, const vshCmd *cmd) return ret; } +static int +vshStoragePoolSorter(const void *a, const void *b) +{ + virStoragePoolPtr *pa = (virStoragePoolPtr *) a; + virStoragePoolPtr *pb = (virStoragePoolPtr *) b; + + if (*pa && !*pb) + return -1; + + if (!*pa) + return *pb != NULL; + + return vshStrcasecmp(virStoragePoolGetName(*pa), + virStoragePoolGetName(*pb)); +} + +struct vshStoragePoolList { + virStoragePoolPtr *pools; + size_t npools; +}; +typedef struct vshStoragePoolList *vshStoragePoolListPtr; + +static void +vshStoragePoolListFree(vshStoragePoolListPtr list) +{ + int i; + + if (list && list->pools) { + for (i = 0; i < list->npools; i++) { + if (list->pools[i]) + virStoragePoolFree(list->pools[i]); + } + VIR_FREE(list->pools); + } + VIR_FREE(list); +} + +static vshStoragePoolListPtr +vshStoragePoolListCollect(vshControl *ctl, + unsigned int flags) +{ + vshStoragePoolListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virStoragePoolPtr pool; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActivePools = 0; + int nInactivePools = 0; + int nAllPools = 0; + + /* try the list with flags support (0.10.0 and later) */ + if ((ret = virConnectListAllStoragePools(ctl->conn, + &list->pools, + flags)) >= 0) { + list->npools = ret; + goto finished; + } + + /* check if the command is actually supported */ + if (last_error && last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + goto fallback; + } + + 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_STORAGE_POOLS_ACTIVE | + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE); + vshResetLibvirtError(); + if ((ret = virConnectListAllStoragePools(ctl->conn, &list->pools, + newflags)) >= 0) { + list->npools = ret; + goto filter; + } + } + + /* there was an error during the first or second call */ + vshError(ctl, "%s", _("Failed to list pools")); + goto cleanup; + + +fallback: + /* fall back to old method (0.9.13 and older) */ + vshResetLibvirtError(); + + /* There is no way to get the pool type */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE)) { + vshError(ctl, "%s", _("Filtering using --type is not supported " + "by this libvirt")); + goto cleanup; + } + + /* Get the number of active pools */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if ((nActivePools = virConnectNumOfStoragePools(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to get the number of active pools ")); + goto cleanup; + } + } + + /* Get the number of inactive pools */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE)) { + if ((nInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to get the number of inactive pools")); + goto cleanup; + } + } + + nAllPools = nActivePools + nInactivePools; + + if (nAllPools == 0) + return list; + + names = vshMalloc(ctl, sizeof(char *) * nAllPools); + + /* Retrieve a list of active storage pool names */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if (virConnectListStoragePools(ctl->conn, + names, nActivePools) < 0) { + vshError(ctl, "%s", _("Failed to list active pools")); + goto cleanup; + } + } + + /* Add the inactive storage pools to the end of the name list */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if (virConnectListDefinedStoragePools(ctl->conn, + &names[nActivePools], + nInactivePools) < 0) { + vshError(ctl, "%s", _("Failed to list inactive pools")); + goto cleanup; + } + } + + list->pools = vshMalloc(ctl, sizeof(virStoragePoolPtr) * (nAllPools)); + list->npools = 0; + + /* get active pools */ + for (i = 0; i < nActivePools; i++) { + if (!(pool = virStoragePoolLookupByName(ctl->conn, names[i]))) + continue; + list->pools[list->npools++] = pool; + } + + /* get inactive pools */ + for (i = 0; i < nInactivePools; i++) { + if (!(pool = virStoragePoolLookupByName(ctl->conn, names[i]))) + continue; + list->pools[list->npools++] = pool; + } + + /* truncate pools that weren't found */ + deleted = nAllPools - list->npools; + +filter: + /* filter list the list if the list was acquired by fallback means */ + for (i = 0; i < list->npools; i++) { + pool = list->pools[i]; + + /* persistence filter */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_PERSISTENT)) { + if ((persistent = virStoragePoolIsPersistent(pool)) < 0) { + vshError(ctl, "%s", _("Failed to get pool persistence info")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT) && persistent) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT) && !persistent))) + goto remove_entry; + } + + /* autostart filter */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART)) { + if (virStoragePoolGetAutostart(pool, &autostart) < 0) { + vshError(ctl, "%s", _("Failed to get pool autostart state")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART) && autostart) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART) && !autostart))) + goto remove_entry; + } + + /* the pool matched all filters, it may stay */ + continue; + +remove_entry: + /* the pool has to be removed as it failed one of the filters */ + virStoragePoolFree(list->pools[i]); + list->pools[i] = NULL; + deleted++; + } + +finished: + /* sort the list */ + if (list->pools && list->npools) + qsort(list->pools, list->npools, + sizeof(*list->pools), vshStoragePoolSorter); + + /* truncate the list if filter simulation deleted entries */ + if (deleted) + VIR_SHRINK_N(list->pools, list->npools, deleted); + + success = true; + +cleanup: + for (i = 0; i < nAllPools; i++) + VIR_FREE(names[i]); + + if (!success) { + vshStoragePoolListFree(list); + list = NULL; + } + + VIR_FREE(names); + return list; +} + /* * "pool-list" command */ @@ -563,6 +790,11 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient pools")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent pools")}, + {"autostart", VSH_OT_BOOL, 0, N_("list pools with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list pools with autostart disabled")}, + {"type", VSH_OT_STRING, 0, N_("only list pool of specified type(s) (if supported)")}, {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; @@ -571,7 +803,403 @@ static bool cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { virStoragePoolInfo info; - char **poolNames = NULL; + int i, ret; + bool functionReturn = false; + size_t stringLength = 0, nameStrLength = 0; + size_t autostartStrLength = 0, persistStrLength = 0; + size_t stateStrLength = 0, capStrLength = 0; + size_t allocStrLength = 0, availStrLength = 0; + struct poolInfoText { + char *state; + char *autostart; + char *persistent; + char *capacity; + char *allocation; + char *available; + }; + struct poolInfoText *poolInfoTexts = NULL; + unsigned int flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE; + vshStoragePoolListPtr list = NULL; + const char *type = NULL; + bool details = vshCommandOptBool(cmd, "details"); + bool inactive, all; + + inactive = vshCommandOptBool(cmd, "inactive"); + all = vshCommandOptBool(cmd, "all"); + + if (inactive) + flags = VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE; + + if (all) + flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE; + + if (vshCommandOptBool(cmd, "autostart")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART; + + if (vshCommandOptBool(cmd, "no-autostart")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART; + + if (vshCommandOptBool(cmd, "persistent")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT; + + if (vshCommandOptBool(cmd, "transient")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT; + + if (vshCommandOptString(cmd, "type", &type) < 0) { + vshError(ctl, "%s", _("Invalid argument for 'type'")); + return false; + } + + if (type) { + int poolType = -1; + char **poolTypes = NULL; + int npoolTypes = 0; + + npoolTypes = vshStringToArray((char *)type, &poolTypes); + + for (i = 0; i < npoolTypes; i++) { + if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { + vshError(ctl, "%s", _("Invalid pool type")); + VIR_FREE(poolTypes); + return false; + } + + switch(poolType) { + case VIR_STORAGE_POOL_DIR: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_DIR; + break; + case VIR_STORAGE_POOL_FS: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_FS; + break; + case VIR_STORAGE_POOL_NETFS: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_NETFS; + break; + case VIR_STORAGE_POOL_LOGICAL: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL; + break; + case VIR_STORAGE_POOL_DISK: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_DISK; + break; + case VIR_STORAGE_POOL_ISCSI: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI; + break; + case VIR_STORAGE_POOL_SCSI: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_SCSI; + break; + case VIR_STORAGE_POOL_MPATH: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_MPATH; + break; + case VIR_STORAGE_POOL_RBD: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_RBD; + break; + default: + break; + } + } + VIR_FREE(poolTypes); + } + + if (!(list = vshStoragePoolListCollect(ctl, flags))) + goto cleanup; + + poolInfoTexts = vshCalloc(ctl, list->npools, sizeof(*poolInfoTexts)); + + /* Collect the storage pool information for display */ + for (i = 0; i < list->npools; i++) { + int autostart = 0, persistent = 0; + + /* Retrieve the autostart status of the pool */ + if (virStoragePoolGetAutostart(list->pools[i], &autostart) < 0) + poolInfoTexts[i].autostart = vshStrdup(ctl, _("no autostart")); + else + poolInfoTexts[i].autostart = vshStrdup(ctl, autostart ? + _("yes") : _("no")); + + /* Retrieve the persistence status of the pool */ + if (details) { + persistent = virStoragePoolIsPersistent(list->pools[i]); + vshDebug(ctl, VSH_ERR_DEBUG, "Persistent flag value: %d\n", + persistent); + if (persistent < 0) + poolInfoTexts[i].persistent = vshStrdup(ctl, _("unknown")); + else + poolInfoTexts[i].persistent = vshStrdup(ctl, persistent ? + _("yes") : _("no")); + + /* Keep the length of persistent string if longest so far */ + stringLength = strlen(poolInfoTexts[i].persistent); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + } + + /* Collect further extended information about the pool */ + if (virStoragePoolGetInfo(list->pools[i], &info) != 0) { + /* Something went wrong retrieving pool info, cope with it */ + vshError(ctl, "%s", _("Could not retrieve pool information")); + poolInfoTexts[i].state = vshStrdup(ctl, _("unknown")); + if (details) { + poolInfoTexts[i].capacity = vshStrdup(ctl, _("unknown")); + poolInfoTexts[i].allocation = vshStrdup(ctl, _("unknown")); + poolInfoTexts[i].available = vshStrdup(ctl, _("unknown")); + } + } else { + /* Decide which state string to display */ + if (details) { + /* --details option was specified, we're using detailed state + * strings */ + switch (info.state) { + case VIR_STORAGE_POOL_INACTIVE: + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + break; + case VIR_STORAGE_POOL_BUILDING: + poolInfoTexts[i].state = vshStrdup(ctl, _("building")); + break; + case VIR_STORAGE_POOL_RUNNING: + poolInfoTexts[i].state = vshStrdup(ctl, _("running")); + break; + case VIR_STORAGE_POOL_DEGRADED: + poolInfoTexts[i].state = vshStrdup(ctl, _("degraded")); + break; + case VIR_STORAGE_POOL_INACCESSIBLE: + poolInfoTexts[i].state = vshStrdup(ctl, _("inaccessible")); + break; + } + + /* Create the pool size related strings */ + if (info.state == VIR_STORAGE_POOL_RUNNING || + info.state == VIR_STORAGE_POOL_DEGRADED) { + double val; + const char *unit; + + /* Create the capacity output string */ + val = vshPrettyCapacity(info.capacity, &unit); + ret = virAsprintf(&poolInfoTexts[i].capacity, + "%.2lf %s", val, unit); + if (ret < 0) + /* An error occurred creating the string, return */ + goto asprintf_failure; + + /* Create the allocation output string */ + val = vshPrettyCapacity(info.allocation, &unit); + ret = virAsprintf(&poolInfoTexts[i].allocation, + "%.2lf %s", val, unit); + if (ret < 0) + /* An error occurred creating the string, return */ + goto asprintf_failure; + + /* Create the available space output string */ + val = vshPrettyCapacity(info.available, &unit); + ret = virAsprintf(&poolInfoTexts[i].available, + "%.2lf %s", val, unit); + if (ret < 0) + /* An error occurred creating the string, return */ + goto asprintf_failure; + } else { + /* Capacity related information isn't available */ + poolInfoTexts[i].capacity = vshStrdup(ctl, _("-")); + poolInfoTexts[i].allocation = vshStrdup(ctl, _("-")); + poolInfoTexts[i].available = vshStrdup(ctl, _("-")); + } + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(poolInfoTexts[i].capacity); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(poolInfoTexts[i].allocation); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Keep the length of available string if longest so far */ + stringLength = strlen(poolInfoTexts[i].available); + if (stringLength > availStrLength) + availStrLength = stringLength; + } else { + /* --details option was not specified, only active/inactive + * state strings are used */ + if (virStoragePoolIsActive(list->pools[i])) + poolInfoTexts[i].state = vshStrdup(ctl, _("active")); + else + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + } + } + + /* Keep the length of name string if longest so far */ + stringLength = strlen(virStoragePoolGetName(list->pools[i])); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of state string if longest so far */ + stringLength = strlen(poolInfoTexts[i].state); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Keep the length of autostart string if longest so far */ + stringLength = strlen(poolInfoTexts[i].autostart); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + } + + /* If the --details option wasn't selected, we output the pool + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* Output old style header */ + vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), + _("Autostart")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Output old style pool info */ + for (i = 0; i < list->npools; i++) { + const char *name = virStoragePoolGetName(list->pools[i]); + vshPrint(ctl, "%-20s %-10s %-10s\n", + name, + poolInfoTexts[i].state, + poolInfoTexts[i].autostart); + } + + /* Cleanup and return */ + functionReturn = true; + goto cleanup; + } + + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Use the length of state header string if it's longest */ + stringLength = strlen(_("State")); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Use the length of autostart header string if it's longest */ + stringLength = strlen(_("Autostart")); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Use the length of persistent header string if it's longest */ + stringLength = strlen(_("Persistent")); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Use the length of available header string if it's longest */ + stringLength = strlen(_("Available")); + if (stringLength > availStrLength) + availStrLength = stringLength; + + /* Display the string lengths for debugging. */ + vshDebug(ctl, VSH_ERR_DEBUG, "Longest name string = %lu chars\n", + (unsigned long) nameStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest state string = %lu chars\n", + (unsigned long) stateStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest autostart string = %lu chars\n", + (unsigned long) autostartStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest persistent string = %lu chars\n", + (unsigned long) persistStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest capacity string = %lu chars\n", + (unsigned long) capStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest allocation string = %lu chars\n", + (unsigned long) allocStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest available string = %lu chars\n", + (unsigned long) availStrLength); + + /* Create the output template. Each column is sized according to + * the longest string. + */ + char *outputStr; + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%-%lus %%%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) stateStrLength, + (unsigned long) autostartStrLength, + (unsigned long) persistStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength, + (unsigned long) availStrLength); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("State"), _("Autostart"), + _("Persistent"), _("Capacity"), _("Allocation"), _("Available")); + for (i = nameStrLength + stateStrLength + autostartStrLength + + persistStrLength + capStrLength + + allocStrLength + availStrLength + + 12; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the pool info rows */ + for (i = 0; i < list->npools; i++) { + vshPrint(ctl, outputStr, + virStoragePoolGetName(list->pools[i]), + poolInfoTexts[i].state, + poolInfoTexts[i].autostart, + poolInfoTexts[i].persistent, + poolInfoTexts[i].capacity, + poolInfoTexts[i].allocation, + poolInfoTexts[i].available); + } + + /* Cleanup and return */ + functionReturn = true; + goto cleanup; + +asprintf_failure: + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); + } + functionReturn = false; + +cleanup: + if (list && list->npools) { + for (i = 0; i < list->npools; i++) { + VIR_FREE(poolInfoTexts[i].state); + VIR_FREE(poolInfoTexts[i].autostart); + VIR_FREE(poolInfoTexts[i].persistent); + VIR_FREE(poolInfoTexts[i].capacity); + VIR_FREE(poolInfoTexts[i].allocation); + VIR_FREE(poolInfoTexts[i].available); + } + } + VIR_FREE(poolInfoTexts); + + vshStoragePoolListFree(list); + return functionReturn; +} + +#if 0 +static bool +cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + virStoragePoolInfo info; int i, ret; bool functionReturn; int numActivePools = 0, numInactivePools = 0, numAllPools = 0; @@ -956,6 +1584,7 @@ cleanup: /* Return the desired value */ return functionReturn; } +#endif /* * "find-storage-pool-sources-as" command diff --git a/tools/virsh.pod b/tools/virsh.pod index 98ed4b0..9aeb47e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2185,13 +2185,33 @@ variables, and defaults to C<vi>. Returns basic information about the I<pool> object. -=item B<pool-list> [I<--inactive> | I<--all>] [I<--details>] +=item B<pool-list> [I<--inactive>] [I<--all>] + [I<--persistent>] [I<--transient>] + [I<--autostart>] [I<--no-autostart>] + [[I<--details>] [<type>] List pool objects known to libvirt. By default, only active pools are listed; I<--inactive> lists just the inactive pools, and I<--all> -lists all pools. The I<--details> option instructs virsh to additionally +lists all pools. + +Except the default, I<--inactive>, and I<--all>, you may want to specify more +filtering flags. I<--persistent> is to list the persistent pools, I<--transient> +is to list the transient pools. I<--autostart> is to list the autostarting pools, +I<--no-autostart> is to list the pools with autostarting disabled. + +You may also want to list pools with specified types using I<type>, the +pool types must be separated by comma, e.g. --type dir,disk. The valid pool +types include 'dir', 'fs', 'netfs', 'logical', 'disk', 'iscsi', 'scsi', +'mpath', 'rbd', and 'sheepdog'. + +The I<--details> option instructs virsh to additionally display pool persistence and capacity related information where available. +NOTE: When talking to older servers, this command is forced to use a series of +API calls with an inherent race, where a pool might not be listed or might appear +more than once if it changed state between calls while the list was being +collected. Newer servers do not have this problem. + =item B<pool-name> I<uuid> Convert the I<uuid> to a pool name. -- 1.7.7.3

tools/virsh-pool.c: * vshStoragePoolSorter to sort the pool list by pool name. * struct vshStoragePoolList to present the pool list, pool info is collected by list->poolinfo if 'details' is specified by user. * vshStoragePoolListFree to free the pool list * vshStoragePoolListCollect to collect the pool list, new API virStorageListAllPools is tried first, if it's not supported, fall back to older APIs. * New options --persistent, --transient, --autostart, --no-autostart and --type for pool-list. --persistent or --transient is to filter the returned pool list by whether the pool is persistent or not. --autostart or --no-autostart is to filter the returned pool list by whether the pool is autostarting or not. --type is to filter the pools by pool types. E.g. % virsh pool-list --all --persistent --type dir,disk tools/virsh.pod: * Add documentations for the new options. --- tools/virsh-pool.c | 434 ++++++++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 24 +++- 2 files changed, 358 insertions(+), 100 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index fd239d2..365d035 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h" virStoragePoolPtr vshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, @@ -551,6 +552,232 @@ cmdPoolDumpXML(vshControl *ctl, const vshCmd *cmd) return ret; } +static int +vshStoragePoolSorter(const void *a, const void *b) +{ + virStoragePoolPtr *pa = (virStoragePoolPtr *) a; + virStoragePoolPtr *pb = (virStoragePoolPtr *) b; + + if (*pa && !*pb) + return -1; + + if (!*pa) + return *pb != NULL; + + return vshStrcasecmp(virStoragePoolGetName(*pa), + virStoragePoolGetName(*pb)); +} + +struct vshStoragePoolList { + virStoragePoolPtr *pools; + size_t npools; +}; +typedef struct vshStoragePoolList *vshStoragePoolListPtr; + +static void +vshStoragePoolListFree(vshStoragePoolListPtr list) +{ + int i; + + if (list && list->pools) { + for (i = 0; i < list->npools; i++) { + if (list->pools[i]) + virStoragePoolFree(list->pools[i]); + } + VIR_FREE(list->pools); + } + VIR_FREE(list); +} + +static vshStoragePoolListPtr +vshStoragePoolListCollect(vshControl *ctl, + unsigned int flags) +{ + vshStoragePoolListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virStoragePoolPtr pool; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActivePools = 0; + int nInactivePools = 0; + int nAllPools = 0; + + /* try the list with flags support (0.10.0 and later) */ + if ((ret = virConnectListAllStoragePools(ctl->conn, + &list->pools, + flags)) >= 0) { + list->npools = ret; + goto finished; + } + + /* check if the command is actually supported */ + if (last_error && last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + goto fallback; + } + + 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_STORAGE_POOLS_ACTIVE | + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE); + vshResetLibvirtError(); + if ((ret = virConnectListAllStoragePools(ctl->conn, &list->pools, + newflags)) >= 0) { + list->npools = ret; + goto filter; + } + } + + /* there was an error during the first or second call */ + vshError(ctl, "%s", _("Failed to list pools")); + goto cleanup; + + +fallback: + /* fall back to old method (0.9.13 and older) */ + vshResetLibvirtError(); + + /* There is no way to get the pool type */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE)) { + vshError(ctl, "%s", _("Filtering using --type is not supported " + "by this libvirt")); + goto cleanup; + } + + /* Get the number of active pools */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if ((nActivePools = virConnectNumOfStoragePools(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to get the number of active pools ")); + goto cleanup; + } + } + + /* Get the number of inactive pools */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE)) { + if ((nInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to get the number of inactive pools")); + goto cleanup; + } + } + + nAllPools = nActivePools + nInactivePools; + + if (nAllPools == 0) + return list; + + names = vshMalloc(ctl, sizeof(char *) * nAllPools); + + /* Retrieve a list of active storage pool names */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if (virConnectListStoragePools(ctl->conn, + names, nActivePools) < 0) { + vshError(ctl, "%s", _("Failed to list active pools")); + goto cleanup; + } + } + + /* Add the inactive storage pools to the end of the name list */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if (virConnectListDefinedStoragePools(ctl->conn, + &names[nActivePools], + nInactivePools) < 0) { + vshError(ctl, "%s", _("Failed to list inactive pools")); + goto cleanup; + } + } + + list->pools = vshMalloc(ctl, sizeof(virStoragePoolPtr) * (nAllPools)); + list->npools = 0; + + /* get active pools */ + for (i = 0; i < nActivePools; i++) { + if (!(pool = virStoragePoolLookupByName(ctl->conn, names[i]))) + continue; + list->pools[list->npools++] = pool; + } + + /* get inactive pools */ + for (i = 0; i < nInactivePools; i++) { + if (!(pool = virStoragePoolLookupByName(ctl->conn, names[i]))) + continue; + list->pools[list->npools++] = pool; + } + + /* truncate pools that weren't found */ + deleted = nAllPools - list->npools; + +filter: + /* filter list the list if the list was acquired by fallback means */ + for (i = 0; i < list->npools; i++) { + pool = list->pools[i]; + + /* persistence filter */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_PERSISTENT)) { + if ((persistent = virStoragePoolIsPersistent(pool)) < 0) { + vshError(ctl, "%s", _("Failed to get pool persistence info")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT) && persistent) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT) && !persistent))) + goto remove_entry; + } + + /* autostart filter */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART)) { + if (virStoragePoolGetAutostart(pool, &autostart) < 0) { + vshError(ctl, "%s", _("Failed to get pool autostart state")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART) && autostart) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART) && !autostart))) + goto remove_entry; + } + + /* the pool matched all filters, it may stay */ + continue; + +remove_entry: + /* the pool has to be removed as it failed one of the filters */ + virStoragePoolFree(list->pools[i]); + list->pools[i] = NULL; + deleted++; + } + +finished: + /* sort the list */ + if (list->pools && list->npools) + qsort(list->pools, list->npools, + sizeof(*list->pools), vshStoragePoolSorter); + + /* truncate the list if filter simulation deleted entries */ + if (deleted) + VIR_SHRINK_N(list->pools, list->npools, deleted); + + success = true; + +cleanup: + for (i = 0; i < nAllPools; i++) + VIR_FREE(names[i]); + + if (!success) { + vshStoragePoolListFree(list); + list = NULL; + } + + VIR_FREE(names); + return list; +} + /* * "pool-list" command */ @@ -563,6 +790,11 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient pools")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent pools")}, + {"autostart", VSH_OT_BOOL, 0, N_("list pools with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list pools with autostart disabled")}, + {"type", VSH_OT_STRING, 0, N_("only list pool of specified type(s) (if supported)")}, {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; @@ -571,10 +803,8 @@ static bool cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { virStoragePoolInfo info; - char **poolNames = NULL; int i, ret; - bool functionReturn; - int numActivePools = 0, numInactivePools = 0, numAllPools = 0; + bool functionReturn = false; size_t stringLength = 0, nameStrLength = 0; size_t autostartStrLength = 0, persistStrLength = 0; size_t stateStrLength = 0, capStrLength = 0; @@ -588,80 +818,99 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char *available; }; struct poolInfoText *poolInfoTexts = NULL; - - /* Determine the options passed by the user */ - bool all = vshCommandOptBool(cmd, "all"); + unsigned int flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE; + vshStoragePoolListPtr list = NULL; + const char *type = NULL; bool details = vshCommandOptBool(cmd, "details"); - bool inactive = vshCommandOptBool(cmd, "inactive"); - bool active = !inactive || all; - inactive |= all; + bool inactive, all; - /* Retrieve the number of active storage pools */ - if (active) { - numActivePools = virConnectNumOfStoragePools(ctl->conn); - if (numActivePools < 0) { - vshError(ctl, "%s", _("Failed to list active pools")); - return false; - } - } + inactive = vshCommandOptBool(cmd, "inactive"); + all = vshCommandOptBool(cmd, "all"); - /* Retrieve the number of inactive storage pools */ - if (inactive) { - numInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn); - if (numInactivePools < 0) { - vshError(ctl, "%s", _("Failed to list inactive pools")); - return false; - } - } + if (inactive) + flags = VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE; - /* Determine the total number of pools to list */ - numAllPools = numActivePools + numInactivePools; + if (all) + flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE; - /* Allocate memory for arrays of storage pool names and info */ - poolNames = vshCalloc(ctl, numAllPools, sizeof(*poolNames)); - poolInfoTexts = - vshCalloc(ctl, numAllPools, sizeof(*poolInfoTexts)); + if (vshCommandOptBool(cmd, "autostart")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART; - /* Retrieve a list of active storage pool names */ - if (active) { - if (virConnectListStoragePools(ctl->conn, - poolNames, numActivePools) < 0) { - vshError(ctl, "%s", _("Failed to list active pools")); - VIR_FREE(poolInfoTexts); - VIR_FREE(poolNames); - return false; - } + if (vshCommandOptBool(cmd, "no-autostart")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART; + + if (vshCommandOptBool(cmd, "persistent")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT; + + if (vshCommandOptBool(cmd, "transient")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT; + + if (vshCommandOptString(cmd, "type", &type) < 0) { + vshError(ctl, "%s", _("Invalid argument for 'type'")); + return false; } - /* Add the inactive storage pools to the end of the name list */ - if (inactive) { - if (virConnectListDefinedStoragePools(ctl->conn, - &poolNames[numActivePools], - numInactivePools) < 0) { - vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(poolInfoTexts); - VIR_FREE(poolNames); - return false; + if (type) { + int poolType = -1; + char **poolTypes = NULL; + int npoolTypes = 0; + + npoolTypes = vshStringToArray((char *)type, &poolTypes); + + for (i = 0; i < npoolTypes; i++) { + if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { + vshError(ctl, "%s", _("Invalid pool type")); + VIR_FREE(poolTypes); + return false; + } + + switch(poolType) { + case VIR_STORAGE_POOL_DIR: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_DIR; + break; + case VIR_STORAGE_POOL_FS: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_FS; + break; + case VIR_STORAGE_POOL_NETFS: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_NETFS; + break; + case VIR_STORAGE_POOL_LOGICAL: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL; + break; + case VIR_STORAGE_POOL_DISK: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_DISK; + break; + case VIR_STORAGE_POOL_ISCSI: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI; + break; + case VIR_STORAGE_POOL_SCSI: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_SCSI; + break; + case VIR_STORAGE_POOL_MPATH: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_MPATH; + break; + case VIR_STORAGE_POOL_RBD: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_RBD; + break; + default: + break; + } } + VIR_FREE(poolTypes); } - /* Sort the storage pool names */ - qsort(poolNames, numAllPools, sizeof(*poolNames), vshNameSorter); + if (!(list = vshStoragePoolListCollect(ctl, flags))) + goto cleanup; + + poolInfoTexts = vshCalloc(ctl, list->npools, sizeof(*poolInfoTexts)); /* Collect the storage pool information for display */ - for (i = 0; i < numAllPools; i++) { + for (i = 0; i < list->npools; i++) { int autostart = 0, persistent = 0; - /* Retrieve a pool object, looking it up by name */ - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, - poolNames[i]); - if (!pool) { - VIR_FREE(poolNames[i]); - continue; - } - /* Retrieve the autostart status of the pool */ - if (virStoragePoolGetAutostart(pool, &autostart) < 0) + if (virStoragePoolGetAutostart(list->pools[i], &autostart) < 0) poolInfoTexts[i].autostart = vshStrdup(ctl, _("no autostart")); else poolInfoTexts[i].autostart = vshStrdup(ctl, autostart ? @@ -669,7 +918,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* Retrieve the persistence status of the pool */ if (details) { - persistent = virStoragePoolIsPersistent(pool); + persistent = virStoragePoolIsPersistent(list->pools[i]); vshDebug(ctl, VSH_ERR_DEBUG, "Persistent flag value: %d\n", persistent); if (persistent < 0) @@ -685,7 +934,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* Collect further extended information about the pool */ - if (virStoragePoolGetInfo(pool, &info) != 0) { + if (virStoragePoolGetInfo(list->pools[i], &info) != 0) { /* Something went wrong retrieving pool info, cope with it */ vshError(ctl, "%s", _("Could not retrieve pool information")); poolInfoTexts[i].state = vshStrdup(ctl, _("unknown")); @@ -727,28 +976,25 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) val = vshPrettyCapacity(info.capacity, &unit); ret = virAsprintf(&poolInfoTexts[i].capacity, "%.2lf %s", val, unit); - if (ret < 0) { + if (ret < 0) /* An error occurred creating the string, return */ goto asprintf_failure; - } /* Create the allocation output string */ val = vshPrettyCapacity(info.allocation, &unit); ret = virAsprintf(&poolInfoTexts[i].allocation, "%.2lf %s", val, unit); - if (ret < 0) { + if (ret < 0) /* An error occurred creating the string, return */ goto asprintf_failure; - } /* Create the available space output string */ val = vshPrettyCapacity(info.available, &unit); ret = virAsprintf(&poolInfoTexts[i].available, "%.2lf %s", val, unit); - if (ret < 0) { + if (ret < 0) /* An error occurred creating the string, return */ goto asprintf_failure; - } } else { /* Capacity related information isn't available */ poolInfoTexts[i].capacity = vshStrdup(ctl, _("-")); @@ -772,16 +1018,16 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) availStrLength = stringLength; } else { /* --details option was not specified, only active/inactive - * state strings are used */ - if (info.state == VIR_STORAGE_POOL_INACTIVE) - poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); - else + * state strings are used */ + if (virStoragePoolIsActive(list->pools[i])) poolInfoTexts[i].state = vshStrdup(ctl, _("active")); - } + else + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + } } /* Keep the length of name string if longest so far */ - stringLength = strlen(poolNames[i]); + stringLength = strlen(virStoragePoolGetName(list->pools[i])); if (stringLength > nameStrLength) nameStrLength = stringLength; @@ -794,9 +1040,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) stringLength = strlen(poolInfoTexts[i].autostart); if (stringLength > autostartStrLength) autostartStrLength = stringLength; - - /* Free the pool object */ - virStoragePoolFree(pool); } /* If the --details option wasn't selected, we output the pool @@ -812,9 +1055,10 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshPrintExtra(ctl, "-----------------------------------------\n"); /* Output old style pool info */ - for (i = 0; i < numAllPools; i++) { + for (i = 0; i < list->npools; i++) { + const char *name = virStoragePoolGetName(list->pools[i]); vshPrint(ctl, "%-20s %-10s %-10s\n", - poolNames[i], + name, poolInfoTexts[i].state, poolInfoTexts[i].autostart); } @@ -906,9 +1150,9 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshPrintExtra(ctl, "\n"); /* Display the pool info rows */ - for (i = 0; i < numAllPools; i++) { + for (i = 0; i < list->npools; i++) { vshPrint(ctl, outputStr, - poolNames[i], + virStoragePoolGetName(list->pools[i]), poolInfoTexts[i].state, poolInfoTexts[i].autostart, poolInfoTexts[i].persistent, @@ -922,7 +1166,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) goto cleanup; asprintf_failure: - /* Display an appropriate error message then cleanup and return */ switch (errno) { case ENOMEM: @@ -936,24 +1179,19 @@ asprintf_failure: functionReturn = false; cleanup: - - /* Safely free the memory allocated in this function */ - for (i = 0; i < numAllPools; i++) { - /* Cleanup the memory for one pool info structure */ - VIR_FREE(poolInfoTexts[i].state); - VIR_FREE(poolInfoTexts[i].autostart); - VIR_FREE(poolInfoTexts[i].persistent); - VIR_FREE(poolInfoTexts[i].capacity); - VIR_FREE(poolInfoTexts[i].allocation); - VIR_FREE(poolInfoTexts[i].available); - VIR_FREE(poolNames[i]); + if (list && list->npools) { + for (i = 0; i < list->npools; i++) { + VIR_FREE(poolInfoTexts[i].state); + VIR_FREE(poolInfoTexts[i].autostart); + VIR_FREE(poolInfoTexts[i].persistent); + VIR_FREE(poolInfoTexts[i].capacity); + VIR_FREE(poolInfoTexts[i].allocation); + VIR_FREE(poolInfoTexts[i].available); + } } - - /* Cleanup the memory for the initial arrays*/ VIR_FREE(poolInfoTexts); - VIR_FREE(poolNames); - /* Return the desired value */ + vshStoragePoolListFree(list); return functionReturn; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 98ed4b0..9aeb47e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2185,13 +2185,33 @@ variables, and defaults to C<vi>. Returns basic information about the I<pool> object. -=item B<pool-list> [I<--inactive> | I<--all>] [I<--details>] +=item B<pool-list> [I<--inactive>] [I<--all>] + [I<--persistent>] [I<--transient>] + [I<--autostart>] [I<--no-autostart>] + [[I<--details>] [<type>] List pool objects known to libvirt. By default, only active pools are listed; I<--inactive> lists just the inactive pools, and I<--all> -lists all pools. The I<--details> option instructs virsh to additionally +lists all pools. + +Except the default, I<--inactive>, and I<--all>, you may want to specify more +filtering flags. I<--persistent> is to list the persistent pools, I<--transient> +is to list the transient pools. I<--autostart> is to list the autostarting pools, +I<--no-autostart> is to list the pools with autostarting disabled. + +You may also want to list pools with specified types using I<type>, the +pool types must be separated by comma, e.g. --type dir,disk. The valid pool +types include 'dir', 'fs', 'netfs', 'logical', 'disk', 'iscsi', 'scsi', +'mpath', 'rbd', and 'sheepdog'. + +The I<--details> option instructs virsh to additionally display pool persistence and capacity related information where available. +NOTE: When talking to older servers, this command is forced to use a series of +API calls with an inherent race, where a pool might not be listed or might appear +more than once if it changed state between calls while the list was being +collected. Newer servers do not have this problem. + =item B<pool-name> I<uuid> Convert the I<uuid> to a pool name. -- 1.7.7.3

On 09/05/2012 12:36 AM, Osier Yang wrote:
tools/virsh-pool.c: * vshStoragePoolSorter to sort the pool list by pool name.
* struct vshStoragePoolList to present the pool list, pool info is collected by list->poolinfo if 'details' is specified by user.
* vshStoragePoolListFree to free the pool list
* vshStoragePoolListCollect to collect the pool list, new API virStorageListAllPools is tried first, if it's not supported, fall back to older APIs.
* New options --persistent, --transient, --autostart, --no-autostart and --type for pool-list. --persistent or --transient is to filter the returned pool list by whether the pool is persistent or not. --autostart or --no-autostart is to filter the returned pool list by whether the pool is autostarting or not. --type is to filter the pools by pool types. E.g.
% virsh pool-list --all --persistent --type dir,disk
tools/virsh.pod: * Add documentations for the new options. ---
If you rename things to VSH_MATCH in 8/10, then this patch will be impacted.
+++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h"
I'm not sure if virsh is supposed to be able to use conf/*.h files; you're not the first offender, but the more we do this, the more we are admitting that our public API is insufficient. I'm wondering if we should move the filter group constants into libvirt.h, and make them part of the public API... Regardless of what we decide about the above, though, I think it would be independent cleanup patches and doesn't affect this patch.
+static vshStoragePoolListPtr +vshStoragePoolListCollect(vshControl *ctl, + unsigned int flags) +{ + vshStoragePoolListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virStoragePoolPtr pool; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActivePools = 0; + int nInactivePools = 0; + int nAllPools = 0; + + /* try the list with flags support (0.10.0 and later) */
0.10.2
+ + if (last_error && last_error->code == VIR_ERR_INVALID_ARG) {
Why two spaces after ==?
+ +fallback: + /* fall back to old method (0.9.13 and older) */
0.10.1
@@ -563,6 +790,11 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient pools")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent pools")}, + {"autostart", VSH_OT_BOOL, 0, N_("list pools with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list pools with autostart disabled")}, + {"type", VSH_OT_STRING, 0, N_("only list pool of specified type(s) (if supported)")},
Missing a plural, and the parenthetical '(if supported)' doesn't add any value. Maybe: N_("filter pools by type (multiple types separated by commas)")
+ if (type) { + int poolType = -1; + char **poolTypes = NULL; + int npoolTypes = 0; + + npoolTypes = vshStringToArray((char *)type, &poolTypes); + + for (i = 0; i < npoolTypes; i++) { + if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { + vshError(ctl, "%s", _("Invalid pool type"));
Hmm. What happens if we add new pool types in the future? If libvirt 0.10.3 supports the new pool type 'foo', but I am connecting with virsh 0.10.2 as the client, then I am unable to specify the pool type, even though it is valid to the server. But I don't have any good suggestions on how to make the full list of supported pool types available, and which bits they map to, unless we do something like add a new API to return a list of supported pool types and then have this virsh function consult that list instead of hard-coding the list at the time of the libvirt release when virsh was compiled. Probably not worth the effort.
+ case VIR_STORAGE_POOL_RBD: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_RBD; + break; + default: + break;
If we hit the default case, that means that conf/storage_conf.h has added new pool types but we forgot to update this case statement to cope with them. I think you want to raise an error here instead of doing silent fallthrough, so that we can diagnose such an issue.
+++ b/tools/virsh.pod @@ -2185,13 +2185,33 @@ variables, and defaults to C<vi>.
Returns basic information about the I<pool> object.
-=item B<pool-list> [I<--inactive> | I<--all>] [I<--details>] +=item B<pool-list> [I<--inactive>] [I<--all>] + [I<--persistent>] [I<--transient>] + [I<--autostart>] [I<--no-autostart>] + [[I<--details>] [<type>]
List pool objects known to libvirt. By default, only active pools are listed; I<--inactive> lists just the inactive pools, and I<--all> -lists all pools. The I<--details> option instructs virsh to additionally +lists all pools. + +Except the default, I<--inactive>, and I<--all>, you may want to specify more
Reads awkwardly. How about: In addition, there are several sets of
+filtering flags. I<--persistent> is to list the persistent pools, I<--transient> +is to list the transient pools. I<--autostart> is to list the autostarting pools, +I<--no-autostart> is to list the pools with autostarting disabled.
s/is to list/lists/g I think the things that I pointed out are either too big (and would deserve a separate patch if we decide to do anything at all), or are easily fixable, so I'm okay giving: ACK with findings fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 09/05/2012 10:52 AM, Eric Blake wrote:
+ for (i = 0; i < npoolTypes; i++) { + if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { + vshError(ctl, "%s", _("Invalid pool type"));
Hmm. What happens if we add new pool types in the future? If libvirt 0.10.3 supports the new pool type 'foo', but I am connecting with virsh 0.10.2 as the client, then I am unable to specify the pool type, even though it is valid to the server. But I don't have any good suggestions on how to make the full list of supported pool types available, and which bits they map to, unless we do something like add a new API to return a list of supported pool types and then have this virsh function consult that list instead of hard-coding the list at the time of the libvirt release when virsh was compiled. Probably not worth the effort.
Or maybe instead of a new API, we reuse existing capability XML to expose the full list of supported pool types as a capability. I know I'd also like capability XML to show the supported names of domxml-{to,from}-native transformations possible for a given driver. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 09/05/2012 12:52 PM, Eric Blake wrote:
+++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h" I'm not sure if virsh is supposed to be able to use conf/*.h files; you're not the first offender, but the more we do this, the more we are admitting that our public API is insufficient. I'm wondering if we should move the filter group constants into libvirt.h, and make them
On 09/05/2012 12:36 AM, Osier Yang wrote: part of the public API...
Yes. (or whatever it takes to not use non-public .h files in virsh). virsh should only use the public libvirt API; if it needs something that's private to libvirt, either that piece of code should be rewritten, or the public API should be enhanced. (But, as you say, Osier isn't the first offender, so it's okay to let this temporarily slip by).

On 2012年09月06日 01:17, Laine Stump wrote:
On 09/05/2012 12:52 PM, Eric Blake wrote:
+++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h" I'm not sure if virsh is supposed to be able to use conf/*.h files; you're not the first offender, but the more we do this, the more we are admitting that our public API is insufficient. I'm wondering if we should move the filter group constants into libvirt.h, and make them
On 09/05/2012 12:36 AM, Osier Yang wrote: part of the public API...
For the constants, yeah, we have to move them to public if not want to include the conf headers in client side.
Yes. (or whatever it takes to not use non-public .h files in virsh). virsh should only use the public libvirt API; if it needs something that's private to libvirt, either that piece of code should be rewritten, or the public API should be enhanced.
Agreed for sure, I didn't notice that when creating the patches though. (But, as you say, Osier
isn't the first offender, so it's okay to let this temporarily slip by).
Thanks, I will push the set, and see if can I create a later patch to fix that.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 2012年09月06日 00:52, Eric Blake wrote:
On 09/05/2012 12:36 AM, Osier Yang wrote:
tools/virsh-pool.c: * vshStoragePoolSorter to sort the pool list by pool name.
* struct vshStoragePoolList to present the pool list, pool info is collected by list->poolinfo if 'details' is specified by user.
* vshStoragePoolListFree to free the pool list
* vshStoragePoolListCollect to collect the pool list, new API virStorageListAllPools is tried first, if it's not supported, fall back to older APIs.
* New options --persistent, --transient, --autostart, --no-autostart and --type for pool-list. --persistent or --transient is to filter the returned pool list by whether the pool is persistent or not. --autostart or --no-autostart is to filter the returned pool list by whether the pool is autostarting or not. --type is to filter the pools by pool types. E.g.
% virsh pool-list --all --persistent --type dir,disk
tools/virsh.pod: * Add documentations for the new options. ---
If you rename things to VSH_MATCH in 8/10, then this patch will be impacted.
+++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h"
I'm not sure if virsh is supposed to be able to use conf/*.h files; you're not the first offender, but the more we do this, the more we are admitting that our public API is insufficient. I'm wondering if we should move the filter group constants into libvirt.h, and make them part of the public API...
Regardless of what we decide about the above, though, I think it would be independent cleanup patches and doesn't affect this patch.
+static vshStoragePoolListPtr +vshStoragePoolListCollect(vshControl *ctl, + unsigned int flags) +{ + vshStoragePoolListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virStoragePoolPtr pool; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActivePools = 0; + int nInactivePools = 0; + int nAllPools = 0; + + /* try the list with flags support (0.10.0 and later) */
0.10.2
Updated when pushing.
+ + if (last_error&& last_error->code == VIR_ERR_INVALID_ARG) {
Why two spaces after ==?
+ +fallback: + /* fall back to old method (0.9.13 and older) */
0.10.1
Updated when pushing.
@@ -563,6 +790,11 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive& active pools")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient pools")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent pools")}, + {"autostart", VSH_OT_BOOL, 0, N_("list pools with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list pools with autostart disabled")}, + {"type", VSH_OT_STRING, 0, N_("only list pool of specified type(s) (if supported)")},
Missing a plural, and the parenthetical '(if supported)' doesn't add any value. Maybe:
N_("filter pools by type (multiple types separated by commas)")
Likewise.
+ if (type) { + int poolType = -1; + char **poolTypes = NULL; + int npoolTypes = 0; + + npoolTypes = vshStringToArray((char *)type,&poolTypes); + + for (i = 0; i< npoolTypes; i++) { + if ((poolType = virStoragePoolTypeFromString(poolTypes[i]))< 0) { + vshError(ctl, "%s", _("Invalid pool type"));
Hmm. What happens if we add new pool types in the future? If libvirt 0.10.3 supports the new pool type 'foo', but I am connecting with virsh 0.10.2 as the client, then I am unable to specify the pool type, even though it is valid to the server. But I don't have any good suggestions on how to make the full list of supported pool types available, and which bits they map to, unless we do something like add a new API to return a list of supported pool types and then have this virsh function consult that list instead of hard-coding the list at the time of the libvirt release when virsh was compiled. Probably not worth the effort.
+ case VIR_STORAGE_POOL_RBD: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_RBD; + break; + default: + break;
If we hit the default case, that means that conf/storage_conf.h has added new pool types but we forgot to update this case statement to cope with them. I think you want to raise an error here instead of doing silent fallthrough, so that we can diagnose such an issue.
Added ... vshError(ctl, "%s", _("Unknown pool type")); ... when pushing.
+++ b/tools/virsh.pod @@ -2185,13 +2185,33 @@ variables, and defaults to C<vi>.
Returns basic information about the I<pool> object.
-=item B<pool-list> [I<--inactive> | I<--all>] [I<--details>] +=item B<pool-list> [I<--inactive>] [I<--all>] + [I<--persistent>] [I<--transient>] + [I<--autostart>] [I<--no-autostart>] + [[I<--details>] [<type>]
List pool objects known to libvirt. By default, only active pools are listed; I<--inactive> lists just the inactive pools, and I<--all> -lists all pools. The I<--details> option instructs virsh to additionally +lists all pools. + +Except the default, I<--inactive>, and I<--all>, you may want to specify more
Reads awkwardly. How about:
In addition, there are several sets of
Updated when pushing.
+filtering flags. I<--persistent> is to list the persistent pools, I<--transient> +is to list the transient pools. I<--autostart> is to list the autostarting pools, +I<--no-autostart> is to list the pools with autostarting disabled.
s/is to list/lists/g
Likewise.
I think the things that I pointed out are either too big (and would deserve a separate patch if we decide to do anything at all), or are easily fixable, so I'm okay giving:
ACK with findings fixed.

Please ignore this, and review v5 instead. (I forgot to cleanup the "#if 0...#endif") On 2012年09月04日 23:16, Osier Yang wrote:
tools/virsh-pool.c: * vshStoragePoolSorter to sort the pool list by pool name.
* struct vshStoragePoolList to present the pool list, pool info is collected by list->poolinfo if 'details' is specified by user.
* vshStoragePoolListFree to free the pool list
* vshStoragePoolListCollect to collect the pool list, new API virStorageListAllPools is tried first, if it's not supported, fall back to older APIs.
* New options --persistent, --transient, --autostart, --no-autostart and --type for pool-list. --persistent or --transient is to filter the returned pool list by whether the pool is persistent or not. --autostart or --no-autostart is to filter the returned pool list by whether the pool is autostarting or not. --type is to filter the pools by pool types. E.g.
% virsh pool-list --all --persistent --type dir,disk
tools/virsh.pod: * Add documentations for the new options. --- tools/virsh-pool.c | 631 +++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 24 ++- 2 files changed, 652 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index fd239d2..0b328cc 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/storage_conf.h"
virStoragePoolPtr vshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, @@ -551,6 +552,232 @@ cmdPoolDumpXML(vshControl *ctl, const vshCmd *cmd) return ret; }
+static int +vshStoragePoolSorter(const void *a, const void *b) +{ + virStoragePoolPtr *pa = (virStoragePoolPtr *) a; + virStoragePoolPtr *pb = (virStoragePoolPtr *) b; + + if (*pa&& !*pb) + return -1; + + if (!*pa) + return *pb != NULL; + + return vshStrcasecmp(virStoragePoolGetName(*pa), + virStoragePoolGetName(*pb)); +} + +struct vshStoragePoolList { + virStoragePoolPtr *pools; + size_t npools; +}; +typedef struct vshStoragePoolList *vshStoragePoolListPtr; + +static void +vshStoragePoolListFree(vshStoragePoolListPtr list) +{ + int i; + + if (list&& list->pools) { + for (i = 0; i< list->npools; i++) { + if (list->pools[i]) + virStoragePoolFree(list->pools[i]); + } + VIR_FREE(list->pools); + } + VIR_FREE(list); +} + +static vshStoragePoolListPtr +vshStoragePoolListCollect(vshControl *ctl, + unsigned int flags) +{ + vshStoragePoolListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virStoragePoolPtr pool; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActivePools = 0; + int nInactivePools = 0; + int nAllPools = 0; + + /* try the list with flags support (0.10.0 and later) */ + if ((ret = virConnectListAllStoragePools(ctl->conn, +&list->pools, + flags))>= 0) { + list->npools = ret; + goto finished; + } + + /* check if the command is actually supported */ + if (last_error&& last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + goto fallback; + } + + 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_STORAGE_POOLS_ACTIVE | + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE); + vshResetLibvirtError(); + if ((ret = virConnectListAllStoragePools(ctl->conn,&list->pools, + newflags))>= 0) { + list->npools = ret; + goto filter; + } + } + + /* there was an error during the first or second call */ + vshError(ctl, "%s", _("Failed to list pools")); + goto cleanup; + + +fallback: + /* fall back to old method (0.9.13 and older) */ + vshResetLibvirtError(); + + /* There is no way to get the pool type */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE)) { + vshError(ctl, "%s", _("Filtering using --type is not supported " + "by this libvirt")); + goto cleanup; + } + + /* Get the number of active pools */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if ((nActivePools = virConnectNumOfStoragePools(ctl->conn))< 0) { + vshError(ctl, "%s", _("Failed to get the number of active pools ")); + goto cleanup; + } + } + + /* Get the number of inactive pools */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE)) { + if ((nInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn))< 0) { + vshError(ctl, "%s", _("Failed to get the number of inactive pools")); + goto cleanup; + } + } + + nAllPools = nActivePools + nInactivePools; + + if (nAllPools == 0) + return list; + + names = vshMalloc(ctl, sizeof(char *) * nAllPools); + + /* Retrieve a list of active storage pool names */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if (virConnectListStoragePools(ctl->conn, + names, nActivePools)< 0) { + vshError(ctl, "%s", _("Failed to list active pools")); + goto cleanup; + } + } + + /* Add the inactive storage pools to the end of the name list */ + if (!MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)) { + if (virConnectListDefinedStoragePools(ctl->conn, +&names[nActivePools], + nInactivePools)< 0) { + vshError(ctl, "%s", _("Failed to list inactive pools")); + goto cleanup; + } + } + + list->pools = vshMalloc(ctl, sizeof(virStoragePoolPtr) * (nAllPools)); + list->npools = 0; + + /* get active pools */ + for (i = 0; i< nActivePools; i++) { + if (!(pool = virStoragePoolLookupByName(ctl->conn, names[i]))) + continue; + list->pools[list->npools++] = pool; + } + + /* get inactive pools */ + for (i = 0; i< nInactivePools; i++) { + if (!(pool = virStoragePoolLookupByName(ctl->conn, names[i]))) + continue; + list->pools[list->npools++] = pool; + } + + /* truncate pools that weren't found */ + deleted = nAllPools - list->npools; + +filter: + /* filter list the list if the list was acquired by fallback means */ + for (i = 0; i< list->npools; i++) { + pool = list->pools[i]; + + /* persistence filter */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_PERSISTENT)) { + if ((persistent = virStoragePoolIsPersistent(pool))< 0) { + vshError(ctl, "%s", _("Failed to get pool persistence info")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT)&& persistent) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT)&& !persistent))) + goto remove_entry; + } + + /* autostart filter */ + if (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART)) { + if (virStoragePoolGetAutostart(pool,&autostart)< 0) { + vshError(ctl, "%s", _("Failed to get pool autostart state")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART)&& autostart) || + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART)&& !autostart))) + goto remove_entry; + } + + /* the pool matched all filters, it may stay */ + continue; + +remove_entry: + /* the pool has to be removed as it failed one of the filters */ + virStoragePoolFree(list->pools[i]); + list->pools[i] = NULL; + deleted++; + } + +finished: + /* sort the list */ + if (list->pools&& list->npools) + qsort(list->pools, list->npools, + sizeof(*list->pools), vshStoragePoolSorter); + + /* truncate the list if filter simulation deleted entries */ + if (deleted) + VIR_SHRINK_N(list->pools, list->npools, deleted); + + success = true; + +cleanup: + for (i = 0; i< nAllPools; i++) + VIR_FREE(names[i]); + + if (!success) { + vshStoragePoolListFree(list); + list = NULL; + } + + VIR_FREE(names); + return list; +} + /* * "pool-list" command */ @@ -563,6 +790,11 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive& active pools")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient pools")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent pools")}, + {"autostart", VSH_OT_BOOL, 0, N_("list pools with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list pools with autostart disabled")}, + {"type", VSH_OT_STRING, 0, N_("only list pool of specified type(s) (if supported)")}, {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; @@ -571,7 +803,403 @@ static bool cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { virStoragePoolInfo info; - char **poolNames = NULL; + int i, ret; + bool functionReturn = false; + size_t stringLength = 0, nameStrLength = 0; + size_t autostartStrLength = 0, persistStrLength = 0; + size_t stateStrLength = 0, capStrLength = 0; + size_t allocStrLength = 0, availStrLength = 0; + struct poolInfoText { + char *state; + char *autostart; + char *persistent; + char *capacity; + char *allocation; + char *available; + }; + struct poolInfoText *poolInfoTexts = NULL; + unsigned int flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE; + vshStoragePoolListPtr list = NULL; + const char *type = NULL; + bool details = vshCommandOptBool(cmd, "details"); + bool inactive, all; + + inactive = vshCommandOptBool(cmd, "inactive"); + all = vshCommandOptBool(cmd, "all"); + + if (inactive) + flags = VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE; + + if (all) + flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | + VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE; + + if (vshCommandOptBool(cmd, "autostart")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART; + + if (vshCommandOptBool(cmd, "no-autostart")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART; + + if (vshCommandOptBool(cmd, "persistent")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT; + + if (vshCommandOptBool(cmd, "transient")) + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT; + + if (vshCommandOptString(cmd, "type",&type)< 0) { + vshError(ctl, "%s", _("Invalid argument for 'type'")); + return false; + } + + if (type) { + int poolType = -1; + char **poolTypes = NULL; + int npoolTypes = 0; + + npoolTypes = vshStringToArray((char *)type,&poolTypes); + + for (i = 0; i< npoolTypes; i++) { + if ((poolType = virStoragePoolTypeFromString(poolTypes[i]))< 0) { + vshError(ctl, "%s", _("Invalid pool type")); + VIR_FREE(poolTypes); + return false; + } + + switch(poolType) { + case VIR_STORAGE_POOL_DIR: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_DIR; + break; + case VIR_STORAGE_POOL_FS: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_FS; + break; + case VIR_STORAGE_POOL_NETFS: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_NETFS; + break; + case VIR_STORAGE_POOL_LOGICAL: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_LOGICAL; + break; + case VIR_STORAGE_POOL_DISK: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_DISK; + break; + case VIR_STORAGE_POOL_ISCSI: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_ISCSI; + break; + case VIR_STORAGE_POOL_SCSI: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_SCSI; + break; + case VIR_STORAGE_POOL_MPATH: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_MPATH; + break; + case VIR_STORAGE_POOL_RBD: + flags |= VIR_CONNECT_LIST_STORAGE_POOLS_RBD; + break; + default: + break; + } + } + VIR_FREE(poolTypes); + } + + if (!(list = vshStoragePoolListCollect(ctl, flags))) + goto cleanup; + + poolInfoTexts = vshCalloc(ctl, list->npools, sizeof(*poolInfoTexts)); + + /* Collect the storage pool information for display */ + for (i = 0; i< list->npools; i++) { + int autostart = 0, persistent = 0; + + /* Retrieve the autostart status of the pool */ + if (virStoragePoolGetAutostart(list->pools[i],&autostart)< 0) + poolInfoTexts[i].autostart = vshStrdup(ctl, _("no autostart")); + else + poolInfoTexts[i].autostart = vshStrdup(ctl, autostart ? + _("yes") : _("no")); + + /* Retrieve the persistence status of the pool */ + if (details) { + persistent = virStoragePoolIsPersistent(list->pools[i]); + vshDebug(ctl, VSH_ERR_DEBUG, "Persistent flag value: %d\n", + persistent); + if (persistent< 0) + poolInfoTexts[i].persistent = vshStrdup(ctl, _("unknown")); + else + poolInfoTexts[i].persistent = vshStrdup(ctl, persistent ? + _("yes") : _("no")); + + /* Keep the length of persistent string if longest so far */ + stringLength = strlen(poolInfoTexts[i].persistent); + if (stringLength> persistStrLength) + persistStrLength = stringLength; + } + + /* Collect further extended information about the pool */ + if (virStoragePoolGetInfo(list->pools[i],&info) != 0) { + /* Something went wrong retrieving pool info, cope with it */ + vshError(ctl, "%s", _("Could not retrieve pool information")); + poolInfoTexts[i].state = vshStrdup(ctl, _("unknown")); + if (details) { + poolInfoTexts[i].capacity = vshStrdup(ctl, _("unknown")); + poolInfoTexts[i].allocation = vshStrdup(ctl, _("unknown")); + poolInfoTexts[i].available = vshStrdup(ctl, _("unknown")); + } + } else { + /* Decide which state string to display */ + if (details) { + /* --details option was specified, we're using detailed state + * strings */ + switch (info.state) { + case VIR_STORAGE_POOL_INACTIVE: + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + break; + case VIR_STORAGE_POOL_BUILDING: + poolInfoTexts[i].state = vshStrdup(ctl, _("building")); + break; + case VIR_STORAGE_POOL_RUNNING: + poolInfoTexts[i].state = vshStrdup(ctl, _("running")); + break; + case VIR_STORAGE_POOL_DEGRADED: + poolInfoTexts[i].state = vshStrdup(ctl, _("degraded")); + break; + case VIR_STORAGE_POOL_INACCESSIBLE: + poolInfoTexts[i].state = vshStrdup(ctl, _("inaccessible")); + break; + } + + /* Create the pool size related strings */ + if (info.state == VIR_STORAGE_POOL_RUNNING || + info.state == VIR_STORAGE_POOL_DEGRADED) { + double val; + const char *unit; + + /* Create the capacity output string */ + val = vshPrettyCapacity(info.capacity,&unit); + ret = virAsprintf(&poolInfoTexts[i].capacity, + "%.2lf %s", val, unit); + if (ret< 0) + /* An error occurred creating the string, return */ + goto asprintf_failure; + + /* Create the allocation output string */ + val = vshPrettyCapacity(info.allocation,&unit); + ret = virAsprintf(&poolInfoTexts[i].allocation, + "%.2lf %s", val, unit); + if (ret< 0) + /* An error occurred creating the string, return */ + goto asprintf_failure; + + /* Create the available space output string */ + val = vshPrettyCapacity(info.available,&unit); + ret = virAsprintf(&poolInfoTexts[i].available, + "%.2lf %s", val, unit); + if (ret< 0) + /* An error occurred creating the string, return */ + goto asprintf_failure; + } else { + /* Capacity related information isn't available */ + poolInfoTexts[i].capacity = vshStrdup(ctl, _("-")); + poolInfoTexts[i].allocation = vshStrdup(ctl, _("-")); + poolInfoTexts[i].available = vshStrdup(ctl, _("-")); + } + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(poolInfoTexts[i].capacity); + if (stringLength> capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(poolInfoTexts[i].allocation); + if (stringLength> allocStrLength) + allocStrLength = stringLength; + + /* Keep the length of available string if longest so far */ + stringLength = strlen(poolInfoTexts[i].available); + if (stringLength> availStrLength) + availStrLength = stringLength; + } else { + /* --details option was not specified, only active/inactive + * state strings are used */ + if (virStoragePoolIsActive(list->pools[i])) + poolInfoTexts[i].state = vshStrdup(ctl, _("active")); + else + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + } + } + + /* Keep the length of name string if longest so far */ + stringLength = strlen(virStoragePoolGetName(list->pools[i])); + if (stringLength> nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of state string if longest so far */ + stringLength = strlen(poolInfoTexts[i].state); + if (stringLength> stateStrLength) + stateStrLength = stringLength; + + /* Keep the length of autostart string if longest so far */ + stringLength = strlen(poolInfoTexts[i].autostart); + if (stringLength> autostartStrLength) + autostartStrLength = stringLength; + } + + /* If the --details option wasn't selected, we output the pool + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* Output old style header */ + vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), + _("Autostart")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Output old style pool info */ + for (i = 0; i< list->npools; i++) { + const char *name = virStoragePoolGetName(list->pools[i]); + vshPrint(ctl, "%-20s %-10s %-10s\n", + name, + poolInfoTexts[i].state, + poolInfoTexts[i].autostart); + } + + /* Cleanup and return */ + functionReturn = true; + goto cleanup; + } + + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength> nameStrLength) + nameStrLength = stringLength; + + /* Use the length of state header string if it's longest */ + stringLength = strlen(_("State")); + if (stringLength> stateStrLength) + stateStrLength = stringLength; + + /* Use the length of autostart header string if it's longest */ + stringLength = strlen(_("Autostart")); + if (stringLength> autostartStrLength) + autostartStrLength = stringLength; + + /* Use the length of persistent header string if it's longest */ + stringLength = strlen(_("Persistent")); + if (stringLength> persistStrLength) + persistStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength> capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength> allocStrLength) + allocStrLength = stringLength; + + /* Use the length of available header string if it's longest */ + stringLength = strlen(_("Available")); + if (stringLength> availStrLength) + availStrLength = stringLength; + + /* Display the string lengths for debugging. */ + vshDebug(ctl, VSH_ERR_DEBUG, "Longest name string = %lu chars\n", + (unsigned long) nameStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest state string = %lu chars\n", + (unsigned long) stateStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest autostart string = %lu chars\n", + (unsigned long) autostartStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest persistent string = %lu chars\n", + (unsigned long) persistStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest capacity string = %lu chars\n", + (unsigned long) capStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest allocation string = %lu chars\n", + (unsigned long) allocStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, "Longest available string = %lu chars\n", + (unsigned long) availStrLength); + + /* Create the output template. Each column is sized according to + * the longest string. + */ + char *outputStr; + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%-%lus %%%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) stateStrLength, + (unsigned long) autostartStrLength, + (unsigned long) persistStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength, + (unsigned long) availStrLength); + if (ret< 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("State"), _("Autostart"), + _("Persistent"), _("Capacity"), _("Allocation"), _("Available")); + for (i = nameStrLength + stateStrLength + autostartStrLength + + persistStrLength + capStrLength + + allocStrLength + availStrLength + + 12; i> 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the pool info rows */ + for (i = 0; i< list->npools; i++) { + vshPrint(ctl, outputStr, + virStoragePoolGetName(list->pools[i]), + poolInfoTexts[i].state, + poolInfoTexts[i].autostart, + poolInfoTexts[i].persistent, + poolInfoTexts[i].capacity, + poolInfoTexts[i].allocation, + poolInfoTexts[i].available); + } + + /* Cleanup and return */ + functionReturn = true; + goto cleanup; + +asprintf_failure: + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); + } + functionReturn = false; + +cleanup: + if (list&& list->npools) { + for (i = 0; i< list->npools; i++) { + VIR_FREE(poolInfoTexts[i].state); + VIR_FREE(poolInfoTexts[i].autostart); + VIR_FREE(poolInfoTexts[i].persistent); + VIR_FREE(poolInfoTexts[i].capacity); + VIR_FREE(poolInfoTexts[i].allocation); + VIR_FREE(poolInfoTexts[i].available); + } + } + VIR_FREE(poolInfoTexts); + + vshStoragePoolListFree(list); + return functionReturn; +} + +#if 0
Oops. I fogot to cleanup this. V5 will be posed. Regards, Osier

The implementation is done manually as the generator does not support wrapping lists of C pointers into Python objects. python/libvirt-override-api.xml: Document python/libvirt-override-virConnect.py: Add listAllStoragePools python/libvirt-override.c: Implementation for the wrapper. --- python/libvirt-override-api.xml | 6 ++++ python/libvirt-override-virConnect.py | 12 ++++++++ python/libvirt-override.c | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 0 deletions(-) diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 67ef36e..d16755c 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -306,6 +306,12 @@ <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> <return type='str *' info='the list of Names of None in case of error'/> </function> + <function name='virConnectListAllStoragePools' file='python'> + <info>returns list of all storage pools</info> + <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor connection'/> + <arg name='flags' type='unsigned int' info='optional flags'/> + <return type='pool *' info='the list of pools or None in case of error'/> + </function> <function name='virStoragePoolListVolumes' file='python'> <info>list the storage volumes, stores the pointers to the names in @names</info> <arg name='pool' type='virStoragePoolPtr' info='pointer to the storage pool'/> diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 50177ab..87a737f 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -206,3 +206,15 @@ retlist.append(virDomain(self, _obj=domptr)) return retlist + + def listAllStoragePools(self, flags): + """Returns a list of storage pool objects""" + ret = libvirtmod.virConnectListAllStoragePools(self._o, flags) + if ret is None: + raise libvirtError("virConnectListAllStoragePools() failed", conn=self) + + retlist = list() + for poolptr in ret: + retlist.append(virStoragePool(self, _obj=poolptr)) + + return retlist diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 65e8c69..1e3ad89 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2983,6 +2983,52 @@ libvirt_virConnectListDefinedStoragePools(PyObject *self ATTRIBUTE_UNUSED, return py_retval; } +static PyObject * +libvirt_virConnectListAllStoragePools(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *pyobj_conn; + PyObject *py_retval = NULL; + PyObject *tmp = NULL; + virConnectPtr conn; + virStoragePoolPtr *pools = NULL; + int c_retval = 0; + int i; + unsigned int flags; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virConnectListAllStoragePools", + &pyobj_conn, &flags)) + return NULL; + conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virConnectListAllStoragePools(conn, &pools, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if (!(tmp = libvirt_virStoragePoolPtrWrap(pools[i])) || + PyList_SetItem(py_retval, i, tmp) < 0) { + Py_XDECREF(tmp); + Py_DECREF(py_retval); + py_retval = NULL; + goto cleanup; + } + /* python steals the pointer */ + pools[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (pools[i]) + virStoragePoolFree(pools[i]); + VIR_FREE(pools); + return py_retval; +} static PyObject * libvirt_virStoragePoolListVolumes(PyObject *self ATTRIBUTE_UNUSED, @@ -5878,6 +5924,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virDomainGetVcpuPinInfo", libvirt_virDomainGetVcpuPinInfo, METH_VARARGS, NULL}, {(char *) "virConnectListStoragePools", libvirt_virConnectListStoragePools, METH_VARARGS, NULL}, {(char *) "virConnectListDefinedStoragePools", libvirt_virConnectListDefinedStoragePools, METH_VARARGS, NULL}, + {(char *) "virConnectListAllStoragePools", libvirt_virConnectListAllStoragePools, METH_VARARGS, NULL}, {(char *) "virStoragePoolGetAutostart", libvirt_virStoragePoolGetAutostart, METH_VARARGS, NULL}, {(char *) "virStoragePoolListVolumes", libvirt_virStoragePoolListVolumes, METH_VARARGS, NULL}, {(char *) "virStoragePoolGetInfo", libvirt_virStoragePoolGetInfo, METH_VARARGS, NULL}, -- 1.7.7.3

On 09/04/2012 09:16 AM, Osier Yang wrote:
The implementation is done manually as the generator does not support wrapping lists of C pointers into Python objects.
Given the number of times we've made comments like this in the overall series, I can't help but wonder if it is worth teaching the generators (RPC and python bindings) about wrapping the listall APIs. But that's icing on top. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年09月04日 23:16, Osier Yang wrote:
Think the subject tells enough after 3 rounds, so ommits words here.
v3 - v4: * Just rebase on the top: - Version changes - rebase after virsh split cleanups
1/10 ~ 2/10 are ACK'ed in v3, but for easy viewing, they are still posted.
Osier Yang (10): list: Define new API virStorageListAllStoragePools list: Add helpers for listing storage pool objects list: Implement the RPC calls for virConnectListAllStoragePools list: Implement listAllStoragePools for storage driver list: Implement listAllStoragePools for test driver list: Add helper to convert strings separated by ', ' to array virsh: Fix the wrong doc for pool-list list: Change MATCH for common use in virsh list: Use virConnectListAllStoragePools in virsh python: Expose virStorageListAllStoragePools to python binding
daemon/remote.c | 54 +++ include/libvirt/libvirt.h.in | 33 ++ python/generator.py | 5 +- python/libvirt-override-api.xml | 6 + python/libvirt-override-virConnect.py | 12 + python/libvirt-override.c | 47 +++ src/conf/storage_conf.c | 116 ++++++ src/conf/storage_conf.h | 35 ++ src/driver.h | 5 + src/libvirt.c | 112 ++++++- src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 + src/remote/remote_driver.c | 64 ++++ src/remote/remote_protocol.x | 13 +- src/remote_protocol-structs | 12 + src/storage/storage_driver.c | 18 + src/test/test_driver.c | 17 + tools/virsh-domain-monitor.c | 2 - tools/virsh-domain.c | 19 +- tools/virsh-pool.c | 631 ++++++++++++++++++++++++++++++++- tools/virsh.c | 44 +++ tools/virsh.h | 3 + tools/virsh.pod | 33 ++- 23 files changed, 1249 insertions(+), 38 deletions(-)
Thanks for the reviewing, now I pushed the set with all the pointed out nits fixed. Regards, Osier
participants (3)
-
Eric Blake
-
Laine Stump
-
Osier Yang