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

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 7/23/24 15:34, Abhiram Tilak wrote:
> From: notpua <atp@xxxxxxxxxxxx>
> 
> 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.
> 
> Related Issue: https://gitlab.com/libvirt/libvirt/-/issues/9
> Signed-off-by: Abhiram Tilak <atp.exp@xxxxxxxxx>
> ---
> Changes in v2:
>   - Fix all formatting errors
>   - Change some options using Empty completers, to use
>     LocalPath completers.
>   - Add completers for AdapterName and AdapterParent using information
>     from node devices.
> Changes in v3:
>   - Call virshNodeDeviceNameComplete with modified flags instead of 
>     duplicating the whole implementation.
> 
>  src/libvirt_private.syms     |  2 ++
>  tools/virsh-completer-pool.c | 70 ++++++++++++++++++++++++++++++++++++
>  tools/virsh-completer-pool.h | 20 +++++++++++
>  tools/virsh-pool.c           |  9 +++++
>  4 files changed, 101 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c35366c9e1..6ba1e8e2c5 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1123,6 +1123,8 @@ virStorageAuthDefCopy;
>  virStorageAuthDefFormat;
>  virStorageAuthDefFree;
>  virStorageAuthDefParse;
> +virStorageAuthTypeFromString;
> +virStorageAuthTypeToString;
>  virStorageFileFeatureTypeFromString;
>  virStorageFileFeatureTypeToString;
>  virStorageFileFormatTypeFromString;

This should be in a separate commit. It is a change that's semantically
different to the rest.

> diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c
> index 3568bb985b..ba7855fdba 100644
> --- a/tools/virsh-completer-pool.c
> +++ b/tools/virsh-completer-pool.c
> @@ -23,6 +23,7 @@
>  #include "virsh-completer-pool.h"
>  #include "virsh-util.h"
>  #include "conf/storage_conf.h"
> +#include "conf/node_device_conf.h"
>  #include "virsh-pool.h"
>  #include "virsh.h"
>  
> @@ -106,3 +107,72 @@ virshPoolTypeCompleter(vshControl *ctl,
>  
>      return virshCommaStringListComplete(type_str, (const char **)tmp);
>  }
> +
> +
> +char **
> +virshPoolFormatCompleter(vshControl *ctl G_GNUC_UNUSED,
> +                         const vshCmd *cmd G_GNUC_UNUSED,
> +                         unsigned int flags)
> +{
> +    size_t i = 0;
> +    size_t j = 0;
> +    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;

We don't use TABs. I believe 'meson test' would have reported this.

> +
> +    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 = 1; i < VIR_STORAGE_POOL_DISK_LAST; i++)
> +        tmp[j++] = g_strdup(virStoragePoolFormatDiskTypeToString(i));
> +
> +    for (i = 1; i < VIR_STORAGE_POOL_LOGICAL_LAST; i++)
> +        tmp[j++] = g_strdup(virStoragePoolFormatLogicalTypeToString(i));
> +
> +    return g_steal_pointer(&tmp);
> +}
> +
> +
> +char **
> +virshPoolAuthTypeCompleter(vshControl *ctl G_GNUC_UNUSED,
> +                           const vshCmd *cmd G_GNUC_UNUSED,
> +                           unsigned int flags)
> +{
> +    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));

There's no need to copy internals of virshEnumComplete().

> +
> +    return g_steal_pointer(&tmp);
> +}
> +
> +
> +char **
> +virshAdapterNameCompleter(vshControl *ctl,
> +                          const vshCmd *cmd, unsigned int flags)
> +{
> +    flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST;
> +    return virshNodeDeviceNameCompleter(ctl, cmd, flags);

I get the idea, but this can hardly work, because
virshNodeDeviceNameCompleter() has virCheckFlags(0, NULL);

which means it exits early and returns NULL (i.e. virsh falls back to
file name completion - which is surely not something desired).

Michal