On 6/15/21 2:38 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@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