2:52 a.m.
On 01/22/2018 07:22 PM, John Ferlan wrote:
On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> tools/virsh-completer.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
> tools/virsh-completer.h | 4 ++++
> tools/virsh-pool.c | 28 +++++++++++++--------------
> tools/virsh-volume.c | 42 +++++++++++++++++++++-------------------
> tools/virsh.h | 6 ++++--
> 5 files changed, 95 insertions(+), 36 deletions(-)
>
> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
> index 48dd9fbc2..8ca2fffd9 100644
> --- a/tools/virsh-completer.c
> +++ b/tools/virsh-completer.c
> @@ -147,3 +147,54 @@ virshDomainInterfaceCompleter(vshControl *ctl,
> virStringListFree(ret);
> return NULL;
> }
> +
> +
> +char **
> +virshStoragePoolNameCompleter(vshControl *ctl,
> + const vshCmd *cmd ATTRIBUTE_UNUSED,
> + unsigned int flags)
> +{
> + virshControlPtr priv = ctl->privData;
> + virStoragePoolPtr *pools = NULL;
> + int npools = 0;
> + size_t i = 0;
> + char **ret = NULL;
> +
> + virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE |
> + VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE |
> + VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT |
> + VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT |
> + VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART |
> + VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART,
> + NULL);
So for this and other patches, is this list right? For various storage
consumers, all I see necessary is 0, ACTIVE, INACTIVE, and PERSISTENT.
Oh right. I was writing these in a hurry as I wanted to have them in the
release. But you're right. We don't need all the flags.
For that matter VIRSH_COMMON_OPT_DOMAIN_FULL doesn't seem to use the
full set from virshDomainNameCompleter either... I see 0, ACTIVE,
RUNNING, OTHER, PERSISTENT, PAUSED, and INACTIVE.
Also, it seems there's a dual usage model where either various CONNECT
flags are used or the Completer function could "roll its own"?
Yes. There are no restrictions places on the @flags.
That is
the virshDomainInterfaceCompleter reads the XML files and checks flags
against VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC (e.g. 1 << 0) which is the
same as VIR_CONNECT_LIST_INTERFACES_INACTIVE.
That is just a coincidence. The flags are distinct in meaning, they only
share the same integer value. Just like
VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE and VIR_DOMAIN_STATS_CPU_TOTAL
(both 1 << 0).
A few patches later we
have virshInterfaceNameCompleter that's using the same CONNECT flags.
It could be construed as confusing... Took me a few to realize the usage.
Okay, I can document it. Although I think I covered it in the commit
message for virshDomainInterfaceCompleter. But it's better to have it in
the code apparently.
FWIW: in the IIRC the "odd" thing about MAC's is that they could be
added once the vNIC is active - that is they'll eventually show up. So
just because it's not in the config/inactive XML doesn't mean it won't
be present. So should the code be only reading the XML? Maybe it'd be
better to add a CONNECT flag that returns when a MAC is present and then
let this code choose the active/inactive + new connect mac flag. I
dunno. I know separate issue, patches welcome, yadda yadda, but figured
I'd bring it up.
Hm. I don't think that MACs can be left out from inactive XML. In the
virDomainNetDefParseXML() function, if MAC is not present it's generated
(regardless of the type of vNIC).
> +
> + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
> + return NULL;
Since this same check is copied everywhere, why not move it to the
caller (e.g. vshReadlineParse) ?
Because not every callback needs connection. I mean, yes all current do,
but in theory they don't. For instance "attach-disk --sourcetype" where
we can have only two options ("block", "file") we don't need a
connection.
> +
> + if ((npools = virConnectListAllStoragePools(priv->conn, &pools, flags))
< 0)
> + return NULL;
> +
> + if (VIR_ALLOC_N(ret, npools + 1) < 0)
> + goto error;
> +
> + for (i = 0; i < npools; i++) {
> + const char *name = virStoragePoolGetName(pools[i]);
> +
> + if (VIR_STRDUP(ret[i], name) < 0)
> + goto error;
> +
> + virStoragePoolFree(pools[i]);
> + }
> + VIR_FREE(pools);
> +
> + return ret;
> +
> + error:
> + for (; i < npools; i++)
> + virStoragePoolFree(pools[i]);
> + VIR_FREE(pools);
> + for (i = 0; i < npools; i++)
> + VIR_FREE(ret[i]);
> + VIR_FREE(ret);
> + return NULL;
> +}
> diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
> index 1a2dd685f..249e793b9 100644
> --- a/tools/virsh-completer.h
> +++ b/tools/virsh-completer.h
> @@ -38,4 +38,8 @@ char ** virshDomainInterfaceCompleter(vshControl *ctl,
> const vshCmd *cmd,
> unsigned int flags);
>
> +char ** virshStoragePoolNameCompleter(vshControl *ctl,
> + const vshCmd *cmd,
> + unsigned int flags);
> +
> #endif
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index 094874b64..cea4cfc12 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -34,8 +34,8 @@
> #include "virstring.h"
> #include "virtime.h"
>
> -#define VIRSH_COMMON_OPT_POOL_FULL \
> - VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"))
> +#define VIRSH_COMMON_OPT_POOL_FULL(cflags) \
> + VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), cflags)
>
> #define VIRSH_COMMON_OPT_POOL_BUILD \
> {.name = "build", \
> @@ -182,7 +182,7 @@ static const vshCmdInfo info_pool_autostart[] = {
> };
>
> static const vshCmdOptDef opts_pool_autostart[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = "disable",
> .type = VSH_OT_BOOL,
> @@ -575,7 +575,7 @@ static const vshCmdInfo info_pool_build[] = {
> };
>
> static const vshCmdOptDef opts_pool_build[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
> VIRSH_COMMON_OPT_POOL_NO_OVERWRITE,
> VIRSH_COMMON_OPT_POOL_OVERWRITE,
>
> @@ -625,7 +625,7 @@ static const vshCmdInfo info_pool_destroy[] = {
> };
>
> static const vshCmdOptDef opts_pool_destroy[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE),
>
> {.name = NULL}
> };
> @@ -665,7 +665,7 @@ static const vshCmdInfo info_pool_delete[] = {
> };
>
> static const vshCmdOptDef opts_pool_delete[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
Delete does not work if ACTIVE... Perhaps this should be INACTIVE (which
also I think would mean PERSISTENT).
Ah, good catch.
>
> {.name = NULL}
> };
> @@ -705,7 +705,7 @@ static const vshCmdInfo info_pool_refresh[] = {
> };
>
> static const vshCmdOptDef opts_pool_refresh[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = NULL}
> };
> @@ -745,7 +745,7 @@ static const vshCmdInfo info_pool_dumpxml[] = {
> };
>
> static const vshCmdOptDef opts_pool_dumpxml[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = "inactive",
> .type = VSH_OT_BOOL,
> @@ -1636,7 +1636,7 @@ static const vshCmdInfo info_pool_info[] = {
> };
>
> static const vshCmdOptDef opts_pool_info[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = "bytes",
> .type = VSH_OT_BOOL,
> @@ -1726,7 +1726,7 @@ static const vshCmdInfo info_pool_name[] = {
> };
>
> static const vshCmdOptDef opts_pool_name[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = NULL}
> };
> @@ -1758,7 +1758,7 @@ static const vshCmdInfo info_pool_start[] = {
> };
>
> static const vshCmdOptDef opts_pool_start[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE),
> VIRSH_COMMON_OPT_POOL_BUILD,
> VIRSH_COMMON_OPT_POOL_NO_OVERWRITE,
> VIRSH_COMMON_OPT_POOL_OVERWRITE,
> @@ -1819,7 +1819,7 @@ static const vshCmdInfo info_pool_undefine[] = {
> };
>
> static const vshCmdOptDef opts_pool_undefine[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT),
>
> {.name = NULL}
> };
> @@ -1859,7 +1859,7 @@ static const vshCmdInfo info_pool_uuid[] = {
> };
>
> static const vshCmdOptDef opts_pool_uuid[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = NULL}
> };
> @@ -1896,7 +1896,7 @@ static const vshCmdInfo info_pool_edit[] = {
> };
>
> static const vshCmdOptDef opts_pool_edit[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
>
> {.name = NULL}
> };
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index 8265a3979..b96e205f7 100644
> --- a/tools/virsh-volume.c
> +++ b/tools/virsh-volume.c
> @@ -43,16 +43,18 @@
> #include "virxml.h"
> #include "virstring.h"
>
> -#define VIRSH_COMMON_OPT_POOL_FULL \
> - VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"))
> +#define VIRSH_COMMON_OPT_POOL_FULL(cflags) \
> + VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), cflags)
>
> -#define VIRSH_COMMON_OPT_POOL_NAME \
> - VIRSH_COMMON_OPT_POOL(N_("pool name"))
> +#define VIRSH_COMMON_OPT_POOL_NAME(cflags) \
> + VIRSH_COMMON_OPT_POOL(N_("pool name"), cflags)
>
> -#define VIRSH_COMMON_OPT_POOL_OPTIONAL \
> +#define VIRSH_COMMON_OPT_POOL_OPTIONAL(cflags) \
> {.name = "pool", \
> .type = VSH_OT_STRING, \
> - .help = N_("pool name or uuid") \
> + .help = N_("pool name or uuid"), \
> + .completer = virshStoragePoolNameCompleter, \
> + .completer_flags = cflags, \
> }
>
> #define VIRSH_COMMON_OPT_VOLUME_VOL \
> @@ -165,7 +167,7 @@ static const vshCmdInfo info_vol_create_as[] = {
> };
>
Will any of the vol functions work if the pool is not ACTIVE?
Yeah probably not. I'll change it to ACTIVE. The advantage of listing
all pools is that it's either exactly what we need or a superset of it :-)
Michal