[libvirt] [PATCH 09/12] virsh-volume: Introduce virshStorageVolKeyCompleter

[Michal Prívozník]

On 6/15/21 2:38 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma at suse.com>
> ---
>  tools/virsh-completer-volume.c | 53 ++++++++++++++++++++++++++++++++++
>  tools/virsh-completer-volume.h |  5 ++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/tools/virsh-completer-volume.c b/tools/virsh-completer-volume.c
> index 301ee982a5..14e9069c5a 100644
> --- a/tools/virsh-completer-volume.c
> +++ b/tools/virsh-completer-volume.c
> @@ -69,3 +69,56 @@ virshStorageVolNameCompleter(vshControl *ctl,
>      g_free(vols);
>      return ret;
>  }
> +
> +char **
> +virshStorageVolKeyCompleter(vshControl *ctl,
> +                            const vshCmd *cmd G_GNUC_UNUSED,
> +                            unsigned int flags)
> +{
> +    virshControl *priv = ctl->privData;
> +    struct virshStoragePoolList *list = NULL;
> +    virStorageVolPtr *vols = NULL;
> +    int rc;
> +    int nvols = 0;
> +    size_t i = 0, j = 0, idx = 0;
> +    char **ret = NULL;
> +    g_auto(GStrv) tmp = NULL;
> +
> +    virCheckFlags(0, NULL);
> +
> +    flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE;

I would much rather see this flag passed ...

> +
> +    if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
> +        return NULL;
> +
> +    if (!(list = virshStoragePoolListCollect(ctl, flags)))


.. here directly, instead of overwriting @flags.

> +        goto cleanup;
> +
> +    for (i = 0; i < list->npools; i++) {
> +        if ((rc = virStoragePoolNumOfVolumes(list->pools[i])) < 0)
> +            goto cleanup;
> +        nvols += rc;
> +    }


Alright, so here @nvols contains sum of all volumes from all active
pools. What if at this point a new volume is added into a pool (from a
different connection/outside of libvirt). What I'm getting at is [2].

> +
> +    tmp = g_new0(char *, nvols + 1);
> +
> +    for (i = 0; i < list->npools; i++) {
> +        if ((rc = virStoragePoolListAllVolumes(list->pools[i], &vols, 0)) < 0)
> +            goto cleanup;
> +        for (j = 0; j < rc; j++) {

1: j < rc && idx < nvols

> +            const char *key = virStorageVolGetKey(vols[j]);
> +            tmp[idx++] = g_strdup(key);

2: this is potentially dangerous as idx may grow beyond nvols limit. I
think something like [1] will prevent that. But it won't prevent @vols
and individual virStorageVols from leaking.

> +        }
> +    }
> +
> +    ret = g_steal_pointer(&tmp);
> +
> + cleanup:
> +    for (i = 0; i < list->npools; i++)
> +        if ((rc = virStoragePoolListAllVolumes(list->pools[i], &vols, 0)) > 0)
> +            for (j = 0; j < rc; j++)
> +                virStorageVolFree(vols[j]);

This does not free volumes really. This returns another allocated array
which is freed immediately. But the one allocated above is not freed. I
think this function should be reworked slightly, e.g. like this:

list = virshStoragePoolListCollect(ctl,
VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE);

for (i = 0; i < list->npools; i++) {
  rc = virStoragePoolListAllVolumes(list->pools[i], vols, 0);

  tmp = g_renew(char *, tmp, nvols + rc + 1);
  /* memset new memory to 0 */

  for (j = 0; j < rc; j++) {
   tmp[nvols++] = volume key;
   virStorageVolFree(vols[j]);
  }

  g_free(vols);
}

I've left out error checks, obviously. I'm stopping my review here as
the rest of patches rely on this one.

Michal