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