Re: [libvirt] [PATCH 09/10 v5] list: Use virConnectListAllStoragePools in virsh

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.