[libvirt] [PATCH 1/8] virsh: Introduce virshStoragePoolNameCompleter
John Ferlan
jferlan at redhat.com
Mon Jan 22 18:22:40 UTC 2018
On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn at 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.
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 it's own"? 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. 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.
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.
> +
> + 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) ?
> +
> + 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).
>
> {.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?
John
> static const vshCmdOptDef opts_vol_create_as[] = {
> - VIRSH_COMMON_OPT_POOL_NAME,
> + VIRSH_COMMON_OPT_POOL_NAME(0),
> {.name = "name",
> .type = VSH_OT_DATA,
> .flags = VSH_OFLAG_REQ,
> @@ -378,7 +380,7 @@ static const vshCmdInfo info_vol_create[] = {
> };
>
> static const vshCmdOptDef opts_vol_create[] = {
> - VIRSH_COMMON_OPT_POOL_NAME,
> + VIRSH_COMMON_OPT_POOL_NAME(0),
> VIRSH_COMMON_OPT_FILE(N_("file containing an XML vol description")),
> {.name = "prealloc-metadata",
> .type = VSH_OT_BOOL,
> @@ -440,7 +442,7 @@ static const vshCmdInfo info_vol_create_from[] = {
> };
>
> static const vshCmdOptDef opts_vol_create_from[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
> VIRSH_COMMON_OPT_FILE(N_("file containing an XML vol description")),
> VIRSH_COMMON_OPT_VOLUME_VOL,
> {.name = "inputpool",
> @@ -559,7 +561,7 @@ static const vshCmdOptDef opts_vol_clone[] = {
> .flags = VSH_OFLAG_REQ,
> .help = N_("clone name")
> },
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "prealloc-metadata",
> .type = VSH_OT_BOOL,
> .help = N_("preallocate metadata (for qcow2 instead of full allocation)")
> @@ -651,7 +653,7 @@ static const vshCmdInfo info_vol_upload[] = {
> static const vshCmdOptDef opts_vol_upload[] = {
> VIRSH_COMMON_OPT_VOLUME_VOL,
> VIRSH_COMMON_OPT_FILE(N_("file")),
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "offset",
> .type = VSH_OT_INT,
> .help = N_("volume offset to upload to")
> @@ -766,7 +768,7 @@ static const vshCmdInfo info_vol_download[] = {
> static const vshCmdOptDef opts_vol_download[] = {
> VIRSH_COMMON_OPT_VOLUME_VOL,
> VIRSH_COMMON_OPT_FILE(N_("file")),
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "offset",
> .type = VSH_OT_INT,
> .help = N_("volume offset to download from")
> @@ -875,7 +877,7 @@ static const vshCmdInfo info_vol_delete[] = {
>
> static const vshCmdOptDef opts_vol_delete[] = {
> VIRSH_COMMON_OPT_VOLUME_VOL,
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "delete-snapshots",
> .type = VSH_OT_BOOL,
> .help = N_("delete snapshots associated with volume (must be "
> @@ -925,7 +927,7 @@ static const vshCmdInfo info_vol_wipe[] = {
>
> static const vshCmdOptDef opts_vol_wipe[] = {
> VIRSH_COMMON_OPT_VOLUME_VOL,
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "algorithm",
> .type = VSH_OT_STRING,
> .help = N_("perform selected wiping algorithm")
> @@ -1012,7 +1014,7 @@ static const vshCmdInfo info_vol_info[] = {
>
> static const vshCmdOptDef opts_vol_info[] = {
> VIRSH_COMMON_OPT_VOLUME_VOL,
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "bytes",
> .type = VSH_OT_BOOL,
> .help = N_("sizes are represented in bytes rather than pretty units")
> @@ -1107,7 +1109,7 @@ static const vshCmdOptDef opts_vol_resize[] = {
> .flags = VSH_OFLAG_REQ,
> .help = N_("new capacity for the vol, as scaled integer (default bytes)")
> },
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = "allocate",
> .type = VSH_OT_BOOL,
> .help = N_("allocate the new capacity, rather than leaving it sparse")
> @@ -1199,7 +1201,7 @@ static const vshCmdInfo info_vol_dumpxml[] = {
>
> static const vshCmdOptDef opts_vol_dumpxml[] = {
> VIRSH_COMMON_OPT_VOLUME_VOL,
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = NULL}
> };
>
> @@ -1363,7 +1365,7 @@ static const vshCmdInfo info_vol_list[] = {
> };
>
> static const vshCmdOptDef opts_vol_list[] = {
> - VIRSH_COMMON_OPT_POOL_FULL,
> + VIRSH_COMMON_OPT_POOL_FULL(0),
> {.name = "details",
> .type = VSH_OT_BOOL,
> .help = N_("display extended details for volumes")
> @@ -1705,7 +1707,7 @@ static const vshCmdOptDef opts_vol_key[] = {
> .flags = VSH_OFLAG_REQ,
> .help = N_("volume name or path")
> },
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = NULL}
> };
>
> @@ -1741,7 +1743,7 @@ static const vshCmdOptDef opts_vol_path[] = {
> .flags = VSH_OFLAG_REQ,
> .help = N_("volume name or key")
> },
> - VIRSH_COMMON_OPT_POOL_OPTIONAL,
> + VIRSH_COMMON_OPT_POOL_OPTIONAL(0),
> {.name = NULL}
> };
>
> diff --git a/tools/virsh.h b/tools/virsh.h
> index 528e04558..f2213ebb5 100644
> --- a/tools/virsh.h
> +++ b/tools/virsh.h
> @@ -64,11 +64,13 @@
> /*
> * Common command options
> */
> -# define VIRSH_COMMON_OPT_POOL(_helpstr) \
> +# define VIRSH_COMMON_OPT_POOL(_helpstr, cflags) \
> {.name = "pool", \
> .type = VSH_OT_DATA, \
> .flags = VSH_OFLAG_REQ, \
> - .help = _helpstr \
> + .help = _helpstr, \
> + .completer = virshStoragePoolNameCompleter, \
> + .completer_flags = cflags, \
> }
>
> # define VIRSH_COMMON_OPT_DOMAIN(_helpstr, cflags) \
>
More information about the libvir-list
mailing list