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

[Osier Yang]

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