[libvirt] [PATCH 00/12] virsh bash completion improvement
Lin Ma (12): virsh: Add comp-methods completion to migrate command virsh: Use VIR_ENUM_* for --target argument in cmdNodeSuspend virsh: Add target completion to nodesuspend command virsh: Use VIR_ENUM_* for --target argument in cmdDomPMSuspend virsh: Add target completion to dompmsuspend command virsh: Add format completion to blockcopy command virsh-pool: Add virshPoolTypeCompleter in macro VIRSH_COMMON_OPT_POOL_X_AS virsh-pool: Remove static from virshStoragePoolList{Free,Collect} virsh-volume: Introduce virshStorageVolKeyCompleter virsh-volume: Add macros VIRSH_COMMON_OPT_VOL_* virsh-volume: Apply virshStorageVolKeyCompleter to vol-{name,pool} commands virsh-volume: Apply virshStorageVolNameCompleter to vol-{key,path} commands tools/virsh-completer-domain.c | 37 +++++++++++++++++++++ tools/virsh-completer-domain.h | 11 +++++++ tools/virsh-completer-host.c | 20 ++++++++++++ tools/virsh-completer-host.h | 4 +++ tools/virsh-completer-volume.c | 53 ++++++++++++++++++++++++++++++ tools/virsh-completer-volume.h | 5 +++ tools/virsh-domain.c | 14 ++++---- tools/virsh-domain.h | 1 + tools/virsh-host.c | 18 +++++----- tools/virsh-host.h | 3 ++ tools/virsh-pool.c | 11 ++----- tools/virsh-pool.h | 11 +++++++ tools/virsh-volume.c | 60 ++++++++++++++++------------------ 13 files changed, 193 insertions(+), 55 deletions(-) -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 17 +++++++++++++++++ tools/virsh-completer-domain.h | 5 +++++ tools/virsh-domain.c | 1 + 3 files changed, 23 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 20d503ff09..256ac0b593 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -959,3 +959,20 @@ virshDomainCoreDumpFormatCompleter(vshControl *ctl G_GNUC_UNUSED, return ret; } + + +char ** +virshDomainMigrateCompMethodsCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags) +{ + const char *methods[] = {"xbzrle", "mt", NULL}; + const char *method = NULL; + + virCheckFlags(0, NULL); + + if (vshCommandOptStringQuiet(ctl, cmd, "comp-methods", &method) < 0) + return NULL; + + return virshCommaStringListComplete(method, methods); +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index 4e16d84514..5d56090504 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -127,3 +127,8 @@ char ** virshDomainCoreDumpFormatCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** +virshDomainMigrateCompMethodsCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ecad3a5e5d..0100652e76 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10619,6 +10619,7 @@ static const vshCmdOptDef opts_migrate[] = { }, {.name = "comp-methods", .type = VSH_OT_STRING, + .completer = virshDomainMigrateCompMethodsCompleter, .help = N_("comma separated list of compression methods to be used") }, {.name = "comp-mt-level", -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-host.c | 17 +++++++++-------- tools/virsh-host.h | 3 +++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 1eca0bc231..9d6d2b3645 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -950,6 +950,13 @@ cmdNodeMemStats(vshControl *ctl, const vshCmd *cmd) /* * "nodesuspend" command */ + +VIR_ENUM_IMPL(virNodeSuspendTarget, + VIR_NODE_SUSPEND_TARGET_LAST, + "mem", + "disk", + "hybrid"); + static const vshCmdInfo info_nodesuspend[] = { {.name = "help", .data = N_("suspend the host node for a given time duration") @@ -980,7 +987,7 @@ static bool cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) { const char *target = NULL; - unsigned int suspendTarget; + int suspendTarget; long long duration; virshControl *priv = ctl->privData; @@ -990,13 +997,7 @@ cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptLongLong(ctl, cmd, "duration", &duration) < 0) return false; - if (STREQ(target, "mem")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM; - } else if (STREQ(target, "disk")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_DISK; - } else if (STREQ(target, "hybrid")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_HYBRID; - } else { + if ((suspendTarget = virNodeSuspendTargetTypeFromString(target)) < 0) { vshError(ctl, "%s", _("Invalid target")); return false; } diff --git a/tools/virsh-host.h b/tools/virsh-host.h index 92328c7deb..840f0b4538 100644 --- a/tools/virsh-host.h +++ b/tools/virsh-host.h @@ -21,5 +21,8 @@ #pragma once #include "vsh.h" +#include "virenum.h" extern const vshCmdDef hostAndHypervisorCmds[]; + +VIR_ENUM_DECL(virNodeSuspendTarget); -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-host.c | 20 ++++++++++++++++++++ tools/virsh-completer-host.h | 4 ++++ tools/virsh-host.c | 1 + 3 files changed, 25 insertions(+) diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c index 213f029552..d17bc5f5ce 100644 --- a/tools/virsh-completer-host.c +++ b/tools/virsh-completer-host.c @@ -26,6 +26,7 @@ #include "virstring.h" #include "virxml.h" #include "virutil.h" +#include "virsh-host.h" static char * virshPagesizeNodeToString(xmlNodePtr node) @@ -167,3 +168,22 @@ virshNodeCpuCompleter(vshControl *ctl, return g_steal_pointer(&tmp); } + + +char ** +virshNodeSuspendTargetCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + char **ret = NULL; + size_t i; + + virCheckFlags(0, NULL); + + ret = g_new0(char *, VIR_NODE_SUSPEND_TARGET_LAST + 1); + + for (i = 0; i < VIR_NODE_SUSPEND_TARGET_LAST; i++) + ret[i] = g_strdup(virNodeSuspendTargetTypeToString(i)); + + return ret; +} diff --git a/tools/virsh-completer-host.h b/tools/virsh-completer-host.h index a502216584..88106ec3db 100644 --- a/tools/virsh-completer-host.h +++ b/tools/virsh-completer-host.h @@ -33,3 +33,7 @@ char ** virshCellnoCompleter(vshControl *ctl, char ** virshNodeCpuCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshNodeSuspendTargetCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 9d6d2b3645..f42e995122 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -972,6 +972,7 @@ static const vshCmdOptDef opts_node_suspend[] = { {.name = "target", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, + .completer = virshNodeSuspendTargetCompleter, .help = N_("mem(Suspend-to-RAM), disk(Suspend-to-Disk), " "hybrid(Hybrid-Suspend)") }, -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 10 ++-------- tools/virsh-domain.h | 1 + 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0100652e76..3fb37ea5e6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3472,7 +3472,7 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd) const char *name; bool ret = false; const char *target = NULL; - unsigned int suspendTarget; + int suspendTarget; unsigned long long duration = 0; if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) @@ -3484,13 +3484,7 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) goto cleanup; - if (STREQ(target, "mem")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM; - } else if (STREQ(target, "disk")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_DISK; - } else if (STREQ(target, "hybrid")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_HYBRID; - } else { + if ((suspendTarget = virNodeSuspendTargetTypeFromString(target)) < 0) { vshError(ctl, "%s", _("Invalid target")); goto cleanup; } diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index 0c1cc7a630..b569dd8d91 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -44,3 +44,4 @@ VIR_ENUM_DECL(virDomainProcessSignal); VIR_ENUM_DECL(virDomainLifecycle); VIR_ENUM_DECL(virDomainLifecycleAction); VIR_ENUM_DECL(virDomainCoreDumpFormat); +VIR_ENUM_DECL(virNodeSuspendTarget); -- 2.26.2
On 6/15/21 2:38 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 10 ++-------- tools/virsh-domain.h | 1 + 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0100652e76..3fb37ea5e6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3472,7 +3472,7 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd) const char *name; bool ret = false; const char *target = NULL; - unsigned int suspendTarget; + int suspendTarget; unsigned long long duration = 0;
if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) @@ -3484,13 +3484,7 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) goto cleanup;
- if (STREQ(target, "mem")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM; - } else if (STREQ(target, "disk")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_DISK; - } else if (STREQ(target, "hybrid")) { - suspendTarget = VIR_NODE_SUSPEND_TARGET_HYBRID; - } else { + if ((suspendTarget = virNodeSuspendTargetTypeFromString(target)) < 0) { vshError(ctl, "%s", _("Invalid target")); goto cleanup; } diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index 0c1cc7a630..b569dd8d91 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -44,3 +44,4 @@ VIR_ENUM_DECL(virDomainProcessSignal); VIR_ENUM_DECL(virDomainLifecycle); VIR_ENUM_DECL(virDomainLifecycleAction); VIR_ENUM_DECL(virDomainCoreDumpFormat); +VIR_ENUM_DECL(virNodeSuspendTarget);
This doesn't feel right. I understand why you added it - so that you can use virNodeSuspendTargetTypeFromString() in the hunk above. However, the enum is implemented and declared in virsh-host files. Thus, what this patch should instead is include virsh-host.h from virsh-domain.c. IOW, I'm squashing in the following: diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3fb37ea5e6..de80b1c2d5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -43,6 +43,7 @@ #include "virstring.h" #include "virsh-console.h" #include "virsh-domain-monitor.h" +#include "virsh-host.h" #include "virerror.h" #include "virtime.h" #include "virtypedparam.h" diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index b569dd8d91..0c1cc7a630 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -44,4 +44,3 @@ VIR_ENUM_DECL(virDomainProcessSignal); VIR_ENUM_DECL(virDomainLifecycle); VIR_ENUM_DECL(virDomainLifecycleAction); VIR_ENUM_DECL(virDomainCoreDumpFormat); -VIR_ENUM_DECL(virNodeSuspendTarget); Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3fb37ea5e6..b3aa52b8a8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3453,6 +3453,7 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = { {.name = "target", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, + .completer = virshNodeSuspendTargetCompleter, .help = N_("mem(Suspend-to-RAM), " "disk(Suspend-to-Disk), " "hybrid(Hybrid-Suspend)") -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 20 ++++++++++++++++++++ tools/virsh-completer-domain.h | 6 ++++++ tools/virsh-domain.c | 2 ++ 3 files changed, 28 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 256ac0b593..14e4d95ec3 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -35,6 +35,7 @@ #include "virkeynametable_linux.h" #include "virkeynametable_osx.h" #include "virkeynametable_win32.h" +#include "conf/storage_conf.h" char ** virshDomainNameCompleter(vshControl *ctl, @@ -976,3 +977,22 @@ virshDomainMigrateCompMethodsCompleter(vshControl *ctl, return virshCommaStringListComplete(method, methods); } + + +char ** +virshDomainStorageFileFormatCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + char **ret = NULL; + size_t i; + + virCheckFlags(0, NULL); + + ret = g_new0(char *, VIR_STORAGE_FILE_LAST + 1); + + for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) + ret[i] = g_strdup(virStorageFileFormatTypeToString(i)); + + return ret; +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index 5d56090504..45380906f9 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -132,3 +132,9 @@ char ** virshDomainMigrateCompMethodsCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + + +char ** +virshDomainStorageFileFormatCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b3aa52b8a8..183f53f913 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2235,6 +2235,8 @@ static const vshCmdOptDef opts_blockcopy[] = { }, {.name = "format", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_NONE, + .completer = virshDomainStorageFileFormatCompleter, .help = N_("format of the destination file") }, {.name = "granularity", -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-pool.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index e8d3c33506..34ed86152e 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -64,6 +64,7 @@ {.name = "type", \ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ + .completer = virshPoolTypeCompleter, \ .help = N_("type of the pool") \ }, \ {.name = "print-xml", \ -- 2.26.2
The functions will be used by next patch. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-pool.c | 10 ++-------- tools/virsh-pool.h | 11 +++++++++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 34ed86152e..18f3839a4c 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -835,13 +835,7 @@ virshStoragePoolSorter(const void *a, const void *b) virStoragePoolGetName(*pb)); } -struct virshStoragePoolList { - virStoragePoolPtr *pools; - size_t npools; -}; - -static void -virshStoragePoolListFree(struct virshStoragePoolList *list) +void virshStoragePoolListFree(struct virshStoragePoolList *list) { size_t i; @@ -855,7 +849,7 @@ virshStoragePoolListFree(struct virshStoragePoolList *list) g_free(list); } -static struct virshStoragePoolList * +struct virshStoragePoolList * virshStoragePoolListCollect(vshControl *ctl, unsigned int flags) { diff --git a/tools/virsh-pool.h b/tools/virsh-pool.h index 219f0eea42..d7bacd8731 100644 --- a/tools/virsh-pool.h +++ b/tools/virsh-pool.h @@ -40,3 +40,14 @@ typedef struct virshPoolEventCallback virshPoolEventCallback; extern virshPoolEventCallback virshPoolEventCallbacks[]; extern const vshCmdDef storagePoolCmds[]; + +struct virshStoragePoolList { + virStoragePoolPtr *pools; + size_t npools; +}; + +struct virshStoragePoolList * +virshStoragePoolListCollect(vshControl *ctl, + unsigned int flags); + +void virshStoragePoolListFree(struct virshStoragePoolList *list); -- 2.26.2
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; + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (!(list = virshStoragePoolListCollect(ctl, flags))) + goto cleanup; + + for (i = 0; i < list->npools; i++) { + if ((rc = virStoragePoolNumOfVolumes(list->pools[i])) < 0) + goto cleanup; + nvols += rc; + } + + 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++) { + const char *key = virStorageVolGetKey(vols[j]); + tmp[idx++] = g_strdup(key); + } + } + + 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]); + g_free(vols); + virshStoragePoolListFree(list); + return ret; +} diff --git a/tools/virsh-completer-volume.h b/tools/virsh-completer-volume.h index 6591e13fdf..b41d8f4f3e 100644 --- a/tools/virsh-completer-volume.h +++ b/tools/virsh-completer-volume.h @@ -26,3 +26,8 @@ char ** virshStorageVolNameCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + + +char ** virshStorageVolKeyCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); -- 2.26.2
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
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-volume.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index c7d5ab8c3b..41b366a833 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -54,14 +54,28 @@ .completer_flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE, \ } -#define VIRSH_COMMON_OPT_VOLUME_VOL \ +#define VIRSH_COMMON_OPT_VOL_NAME(_helpstr) \ {.name = "vol", \ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ - .help = N_("vol name, key or path"), \ + .help = _helpstr, \ .completer = virshStorageVolNameCompleter, \ } +#define VIRSH_COMMON_OPT_VOL_KEY(_helpstr) \ + {.name = "vol", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = _helpstr, \ + .completer = virshStorageVolKeyCompleter, \ + } + +#define VIRSH_COMMON_OPT_VOL_FULL \ + VIRSH_COMMON_OPT_VOL_NAME(N_("vol name, key or path")) + +#define VIRSH_COMMON_OPT_VOL_BY_KEY \ + VIRSH_COMMON_OPT_VOL_KEY(N_("volume key or path")) + virStorageVolPtr virshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, @@ -440,7 +454,7 @@ static const vshCmdInfo info_vol_create_from[] = { static const vshCmdOptDef opts_vol_create_from[] = { VIRSH_COMMON_OPT_POOL_FULL, VIRSH_COMMON_OPT_FILE(N_("file containing an XML vol description")), - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, {.name = "inputpool", .type = VSH_OT_STRING, .help = N_("pool name or uuid of the input volume's pool") @@ -550,7 +564,7 @@ static const vshCmdInfo info_vol_clone[] = { }; static const vshCmdOptDef opts_vol_clone[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, {.name = "newname", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -646,7 +660,7 @@ static const vshCmdInfo info_vol_upload[] = { }; static const vshCmdOptDef opts_vol_upload[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, VIRSH_COMMON_OPT_FILE(N_("file")), VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "offset", @@ -768,7 +782,7 @@ static const vshCmdInfo info_vol_download[] = { }; static const vshCmdOptDef opts_vol_download[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, VIRSH_COMMON_OPT_FILE(N_("file")), VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "offset", @@ -889,7 +903,7 @@ static const vshCmdInfo info_vol_delete[] = { }; static const vshCmdOptDef opts_vol_delete[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "delete-snapshots", .type = VSH_OT_BOOL, @@ -939,7 +953,7 @@ static const vshCmdInfo info_vol_wipe[] = { }; static const vshCmdOptDef opts_vol_wipe[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "algorithm", .type = VSH_OT_STRING, @@ -1027,7 +1041,7 @@ static const vshCmdInfo info_vol_info[] = { }; static const vshCmdOptDef opts_vol_info[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "bytes", .type = VSH_OT_BOOL, @@ -1117,7 +1131,7 @@ static const vshCmdInfo info_vol_resize[] = { }; static const vshCmdOptDef opts_vol_resize[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, {.name = "capacity", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1214,7 +1228,7 @@ static const vshCmdInfo info_vol_dumpxml[] = { }; static const vshCmdOptDef opts_vol_dumpxml[] = { - VIRSH_COMMON_OPT_VOLUME_VOL, + VIRSH_COMMON_OPT_VOL_FULL, VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = NULL} }; -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-volume.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 41b366a833..daa205ea67 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -1552,11 +1552,7 @@ static const vshCmdInfo info_vol_name[] = { }; static const vshCmdOptDef opts_vol_name[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("volume key or path") - }, + VIRSH_COMMON_OPT_VOL_BY_KEY, {.name = NULL} }; @@ -1588,11 +1584,7 @@ static const vshCmdInfo info_vol_pool[] = { }; static const vshCmdOptDef opts_vol_pool[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("volume key or path") - }, + VIRSH_COMMON_OPT_VOL_BY_KEY, {.name = "uuid", .type = VSH_OT_BOOL, .help = N_("return the pool uuid rather than pool name") -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-volume.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index daa205ea67..1da9b7217f 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -1643,11 +1643,7 @@ static const vshCmdInfo info_vol_key[] = { }; static const vshCmdOptDef opts_vol_key[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("volume name or path") - }, + VIRSH_COMMON_OPT_VOL_NAME(N_("volume name or path")), VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = NULL} }; @@ -1679,11 +1675,7 @@ static const vshCmdInfo info_vol_path[] = { }; static const vshCmdOptDef opts_vol_path[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("volume name or key") - }, + VIRSH_COMMON_OPT_VOL_NAME(N_("volume name or key")), VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = NULL} }; -- 2.26.2
On 6/15/21 2:38 AM, Lin Ma wrote:
Lin Ma (12): virsh: Add comp-methods completion to migrate command virsh: Use VIR_ENUM_* for --target argument in cmdNodeSuspend virsh: Add target completion to nodesuspend command virsh: Use VIR_ENUM_* for --target argument in cmdDomPMSuspend virsh: Add target completion to dompmsuspend command virsh: Add format completion to blockcopy command virsh-pool: Add virshPoolTypeCompleter in macro VIRSH_COMMON_OPT_POOL_X_AS virsh-pool: Remove static from virshStoragePoolList{Free,Collect} virsh-volume: Introduce virshStorageVolKeyCompleter virsh-volume: Add macros VIRSH_COMMON_OPT_VOL_* virsh-volume: Apply virshStorageVolKeyCompleter to vol-{name,pool} commands virsh-volume: Apply virshStorageVolNameCompleter to vol-{key,path} commands
tools/virsh-completer-domain.c | 37 +++++++++++++++++++++ tools/virsh-completer-domain.h | 11 +++++++ tools/virsh-completer-host.c | 20 ++++++++++++ tools/virsh-completer-host.h | 4 +++ tools/virsh-completer-volume.c | 53 ++++++++++++++++++++++++++++++ tools/virsh-completer-volume.h | 5 +++ tools/virsh-domain.c | 14 ++++---- tools/virsh-domain.h | 1 + tools/virsh-host.c | 18 +++++----- tools/virsh-host.h | 3 ++ tools/virsh-pool.c | 11 ++----- tools/virsh-pool.h | 11 +++++++ tools/virsh-volume.c | 60 ++++++++++++++++------------------ 13 files changed, 193 insertions(+), 55 deletions(-)
ACK to 01-08. However, I'm pushing 01-07 only because 08 refers to 09 which needs to be reworked. Michal
participants (3)
-
Lin Ma -
Michal Privoznik -
Michal Prívozník