Re: [PATCH v2] virsh-completer: modify and fix bug in virshPoolTypeCompleter, now used for more commands
On 2/20/24 10:31, Adam Julis wrote:
Signed-off-by: Adam Julis <ajulis(a)redhat.com>
---
v2:
- instead new completer is used already existing virshPoolTypeCompleter
- added flag VIRSH_POOL_TYPE_COMPLETER_COMMA for the completer
- fixed bug in pool-list --type command
v1:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/K...
tools/virsh-completer-domain.h | 4 ++++
tools/virsh-completer-pool.c | 14 +++++++++-----
tools/virsh-pool.c | 3 +++
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index 27cf963912..fe4af07937 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -31,6 +31,10 @@ enum {
VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC = 1 << 0, /* Return just MACs */
};
+enum {
+ VIRSH_POOL_TYPE_COMPLETER_COMMA = 1 << 0,
+};
+
This doesn't need to be in virsh-completer-domain.h but can live in
virsh-completer-pool.h happilly since its only purpose is to be passed
to virshPoolTypeCompleter() which is declared in the latter header file.
char **
virshDomainInterfaceCompleter(vshControl *ctl,
const vshCmd *cmd,
diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c
index 0600394411..abad0fca1b 100644
--- a/tools/virsh-completer-pool.c
+++ b/tools/virsh-completer-pool.c
@@ -93,13 +93,17 @@ virshPoolTypeCompleter(vshControl *ctl,
g_auto(GStrv) tmp = NULL;
const char *type_str = NULL;
- virCheckFlags(0, NULL);
-
- if (vshCommandOptStringQuiet(ctl, cmd, "type", &type_str) < 0)
- return NULL;
+ virCheckFlags(VIRSH_POOL_TYPE_COMPLETER_COMMA, NULL);
tmp = virshEnumComplete(VIR_STORAGE_POOL_LAST,
virStoragePoolTypeToString);
- return virshCommaStringListComplete(type_str, (const char **)tmp);
+ if (flags & VIRSH_POOL_TYPE_COMPLETER_COMMA){
+ if (vshCommandOptStringQuiet(ctl, cmd, "type", &type_str) < 0)
+ return NULL;
+
+ return virshCommaStringListComplete(type_str, (const char **)tmp);
+ }
+
+ return g_steal_pointer(&tmp);
After playing with this for a while I found it more readable if the code
is reorganized a bit:
if (!(flags & VIRSH_POOL....))
return g_steal_pointer(&tmp);
if (vshCommandOptStringQuiet() < 0)
return NULL;
return virshCommaStringListComplete();
}
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 36f00cf643..6936406087 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -1088,6 +1088,7 @@ static const vshCmdOptDef opts_pool_list[] = {
},
{.name = "type",
.type = VSH_OT_STRING,
+ .completer_flags = VIRSH_POOL_TYPE_COMPLETER_COMMA,
.completer = virshPoolTypeCompleter,
Personal preference - .completer_flags can be declared after .completer.
.help = N_("only list pool of specified type(s) (if
supported)")
},
@@ -1414,6 +1415,7 @@ static const vshCmdOptDef opts_find_storage_pool_sources_as[] = {
{.name = "type",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshPoolTypeCompleter,
.help = N_("type of storage pool sources to find")
},
{.name = "host",
@@ -1501,6 +1503,7 @@ static const vshCmdOptDef opts_find_storage_pool_sources[] = {
{.name = "type",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshPoolTypeCompleter,
.help = N_("type of storage pool sources to discover")
},
{.name = "srcSpec",
I've fixed all the small issues and merged. Congratulations on your
first libvirt contribution!
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal