Re: [libvirt] [PATCH 09/12] virsh-volume: Introduce virshStorageVolKeyCompleter
On 6/15/21 2:38 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma(a)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