Re: [PATCH] virsh: Provide completer for some pool-X-as commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Tue, 28 May 2024 at 17:35, Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
On Mon, May 27, 2024 at 18:34:38 +0000, Abhiram Tilak wrote:
> Provides completers for auth-type and source-format commands for
> virsh pool-create-as and pool-define-as commands. Use Empty completers
> for options where completions are not required. I left the ones where
> I was not sure if they need completers.
>
> Related Issue: https://gitlab.com/libvirt/libvirt/-/issues/9
> Signed-off-by: Abhiram Tilak <atp.exp@xxxxxxxxx>
> ---
>  src/libvirt_private.syms     |  2 ++
>  tools/virsh-completer-pool.c | 48 +++++++++++++++++++++++++++++++++++-
>  tools/virsh-completer-pool.h | 10 ++++++++
>  tools/virsh-pool.c           |  8 ++++++
>  4 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6b6bcc368a..5a413ca832 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1117,6 +1117,8 @@ virStorageAuthDefCopy;
>  virStorageAuthDefFormat;
>  virStorageAuthDefFree;
>  virStorageAuthDefParse;
> +virStorageAuthTypeFromString;
> +virStorageAuthTypeToString;
>  virStorageFileFeatureTypeFromString;
>  virStorageFileFeatureTypeToString;
>  virStorageFileFormatTypeFromString;
> diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c
> index 3568bb985b..4a771d894e 100644
> --- a/tools/virsh-completer-pool.c
> +++ b/tools/virsh-completer-pool.c
> @@ -84,7 +84,6 @@ virshPoolEventNameCompleter(vshControl *ctl G_GNUC_UNUSED,
>      return g_steal_pointer(&tmp);
>  }

> -

Spurious whitespace change.

>  char **
>  virshPoolTypeCompleter(vshControl *ctl,
>                         const vshCmd *cmd,
> @@ -106,3 +105,50 @@ virshPoolTypeCompleter(vshControl *ctl,

>      return virshCommaStringListComplete(type_str, (const char **)tmp);
>  }
> +

Two empty lines between functions in this file.

> +char **
> +virshPoolFormatCompleter(vshControl *ctl G_GNUC_UNUSED,
> +                         const vshCmd *cmd G_GNUC_UNUSED,
> +                         unsigned int flags)
> +{
> +    size_t i = 0, j = 0;

One declaration per line please;

> +    g_auto(GStrv) tmp = NULL;
> +    size_t nformats = VIR_STORAGE_POOL_FS_LAST + VIR_STORAGE_POOL_NETFS_LAST +
> +                   VIR_STORAGE_POOL_DISK_LAST + VIR_STORAGE_POOL_LOGICAL_LAST;
> +
> +    virCheckFlags(0, NULL);
> +
> +    tmp = g_new0(char *, nformats + 1);
> +
> +    /* Club all PoolFormats for completion */
> +    for (i = 0; i < VIR_STORAGE_POOL_FS_LAST; i++)
> +        tmp[j++] = g_strdup(virStoragePoolFormatFileSystemTypeToString(i));
> +
> +    for (i = 0; i < VIR_STORAGE_POOL_NETFS_LAST; i++)
> +        tmp[j++] = g_strdup(virStoragePoolFormatFileSystemNetTypeToString(i));
> +
> +    for (i = 0; i < VIR_STORAGE_POOL_DISK_LAST; i++)
> +        tmp[j++] = g_strdup(virStoragePoolFormatDiskTypeToString(i));

I don't think it makes sense to complete the "unknown" value here ...

Thanks for pointing this out. I totally missed it.
 
> +
> +    for (i = 0; i < VIR_STORAGE_POOL_LOGICAL_LAST; i++)
> +        tmp[j++] = g_strdup(virStoragePoolFormatLogicalTypeToString(i));

... and here.

> +
> +    return g_steal_pointer(&tmp);
> +}
> +
> +char ** virshPoolAuthTypeCompleter(vshControl *ctl G_GNUC_UNUSED,
> +                         const vshCmd *cmd G_GNUC_UNUSED,
> +                         unsigned int flags)

Wrong header formatting style, and two lines between functions please.

> +{
> +    size_t i = 0;
> +    g_auto(GStrv) tmp = NULL;
> +
> +    virCheckFlags(0, NULL);
> +
> +    tmp = g_new0(char *, VIR_STORAGE_AUTH_TYPE_LAST + 1);
> +
> +    for (i = 0; i < VIR_STORAGE_AUTH_TYPE_LAST; i++)
> +        tmp[i] = g_strdup(virStorageAuthTypeToString(i));
> +
> +    return g_steal_pointer(&tmp);
> +}
> diff --git a/tools/virsh-completer-pool.h b/tools/virsh-completer-pool.h
> index bff3e5742b..4a53f99577 100644
> --- a/tools/virsh-completer-pool.h
> +++ b/tools/virsh-completer-pool.h
> @@ -40,3 +40,13 @@ char **
>  virshPoolTypeCompleter(vshControl *ctl,
>                         const vshCmd *cmd,
>                         unsigned int flags);
> +
> +char **
> +virshPoolFormatCompleter(vshControl *ctl,
> +                         const vshCmd *cmd,
> +                         unsigned int flags);
> +
> +char **
> +virshPoolAuthTypeCompleter(vshControl *ctl,
> +                         const vshCmd *cmd,
> +                         unsigned int flags);
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index 67cc1b94cf..1a294bb0f6 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -81,26 +81,32 @@
>      }, \
>      {.name = "source-path", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshCompletePathLocalExisting, \
>       .help = N_("source path for underlying storage") \
>      }, \
>      {.name = "source-dev", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshCompleteEmpty, \

This is taking the path to a block device, so this should complete
existing filenames.

But lot of times the device isn't "locally" available by virsh, for instance
the first example in the docs: https://libvirt.org/formatstorage.html#source-elements

Here the element:
 <device path="iqn.2013-06.com.example:iscsi-pool"/>

We can't expect the completer to predict this, even though giving
PathLocalExisting completer can help show the local ones, I guess
I will add it anyway. Similarly the `target` can also have Path completer.

>       .help = N_("source device for underlying storage") \
>      }, \
>      {.name = "source-name", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshCompleteEmpty, \
>       .help = N_("source name for underlying storage") \
>      }, \
>      {.name = "target", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshCompleteEmpty, \
>       .help = N_("target for underlying storage") \
>      }, \
>      {.name = "source-format", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshPoolFormatCompleter, \
>       .help = N_("format for underlying storage") \
>      }, \
>      {.name = "auth-type", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshPoolAuthTypeCompleter, \
>       .help = N_("auth type to be used for underlying storage") \
>      }, \
>      {.name = "auth-username", \
> @@ -118,6 +124,7 @@
>      }, \
>      {.name = "adapter-name", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshCompleteEmpty, \

This parameter takes a host SCSI adapter name, which can be completed
from data from the nodedev driver.

virshCompleteEmpty is meant exclusively for user options which we can't
complete.

Turns out there is already a completer for it, will just use that, and be more
specific to get only scsi adapter.
 
>       .help = N_("adapter name to be used for underlying storage") \
>      }, \
>      {.name = "adapter-wwnn", \
> @@ -146,6 +153,7 @@
>      }, \
>      {.name = "source-protocol-ver", \
>       .type = VSH_OT_STRING, \
> +     .completer = virshCompleteEmpty, \
>       .help = N_("nfsvers value for NFS pool mount option") \
>      }, \
>      {.name = "source-initiator", \
> --
> 2.34.1
>


There are other formatting errors too that overlooked, will fix soon.
 

--
Abhiram

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux