[PATCH 0/3] virsh: Completers improvements
*** BLURB HERE *** Michal Prívozník (3): virsh: Properly terminate string list in virshDomainInterfaceSourceModeCompleter() virsh: Introduce virshEnumComplete() virsh: Don't open code virshEnumComplete() tools/virsh-completer-domain.c | 147 ++++++-------------------------- tools/virsh-completer-host.c | 11 +-- tools/virsh-completer-nodedev.c | 7 +- tools/virsh-completer-pool.c | 7 +- tools/virsh-completer-volume.c | 11 +-- tools/virsh-completer.c | 27 ++++++ tools/virsh-completer.h | 4 + 7 files changed, 67 insertions(+), 147 deletions(-) -- 2.34.1
A completer must return a NULL terminated list of strings, which means that when dealing with enums, it has to allocate one pointer more than the value of VIR_XXX_LAST. But this is not honoured in virshDomainInterfaceSourceModeCompleter() leading to out of bounds read. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-completer-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 250dd8b21a..9cc27b84cb 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -500,7 +500,7 @@ virshDomainInterfaceSourceModeCompleter(vshControl *ctl G_GNUC_UNUSED, virCheckFlags(0, NULL); - ret = g_new0(char *, VIRSH_DOMAIN_INTERFACE_SOURCE_MODE_LAST); + ret = g_new0(char *, VIRSH_DOMAIN_INTERFACE_SOURCE_MODE_LAST + 1); for (i = 0; i < VIRSH_DOMAIN_INTERFACE_SOURCE_MODE_LAST; i++) ret[i] = g_strdup(virshDomainInterfaceSourceModeTypeToString(i)); -- 2.34.1
We have plenty of completers which iterate over all values of given enum and do nothing more than translate every member into string (using corresponding virXXXTypeToString()). Introduce a convenience function so that callers can pass just VIR_XXX_LAST and virXXXTypeToString and the rest is taken care of. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-completer.c | 27 +++++++++++++++++++++++++++ tools/virsh-completer.h | 4 ++++ 2 files changed, 31 insertions(+) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 3d77be3121..e5610d0fe8 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -57,6 +57,33 @@ */ +/** + * virshEnumComplete: + * @last: The number of element in enum (pass VIR_XXX_LAST) + * @intToStr: integer to string conversion (pass virXXXTypeToString) + * + * Convenient function to generate completions across all values + * of given enum. The enum, or values we want to generate, must + * start at 0 and be continuous until @last. + * + * Returns: string list of completions. + */ +char ** +virshEnumComplete(unsigned int last, + const char *(*intToStr)(int)) +{ + char **ret = NULL; + size_t i; + + ret = g_new0(char *, last + 1); + + for (i = 0; i < last; i++) + ret[i] = g_strdup(intToStr(i)); + + return ret; +} + + /** * virshCommaStringListComplete: * @input: user input so far diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h index 1d7affbb23..131678dfbc 100644 --- a/tools/virsh-completer.h +++ b/tools/virsh-completer.h @@ -32,6 +32,10 @@ #include "virsh-completer-snapshot.h" #include "virsh-completer-volume.h" +char ** +virshEnumComplete(unsigned int last, + const char *(*intToStr)(int)); + char ** virshCommaStringListComplete(const char *input, const char **options); -- 2.34.1
Now that we have a function that generates string list for given enum, let's use that instead of open coding it. Note, after this there are still some 'candidates' left (e.g, virshNetworkEventNameCompleter(), or virshNetworkUpdateCommandCompleter()). These are not converted because either they don't have a convenient int2str function or they don't start from the very beginning of the enum. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-completer-domain.c | 147 ++++++-------------------------- tools/virsh-completer-host.c | 11 +-- tools/virsh-completer-nodedev.c | 7 +- tools/virsh-completer-pool.c | 7 +- tools/virsh-completer-volume.c | 11 +-- 5 files changed, 36 insertions(+), 147 deletions(-) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 9cc27b84cb..d4c877cd04 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -476,17 +476,10 @@ virshDomainInterfaceAddrSourceCompleter(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_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST + 1); - - for (i = 0; i < VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST; i++) - ret[i] = g_strdup(virshDomainInterfaceAddressesSourceTypeToString(i)); - - return ret; + return virshEnumComplete(VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST, + virshDomainInterfaceAddressesSourceTypeToString); } @@ -495,17 +488,10 @@ virshDomainInterfaceSourceModeCompleter(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 *, VIRSH_DOMAIN_INTERFACE_SOURCE_MODE_LAST + 1); - - for (i = 0; i < VIRSH_DOMAIN_INTERFACE_SOURCE_MODE_LAST; i++) - ret[i] = g_strdup(virshDomainInterfaceSourceModeTypeToString(i)); - - return ret; + return virshEnumComplete(VIRSH_DOMAIN_INTERFACE_SOURCE_MODE_LAST, + virshDomainInterfaceSourceModeTypeToString); } @@ -514,17 +500,10 @@ virshDomainHostnameSourceCompleter(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 *, VIRSH_DOMAIN_HOSTNAME_SOURCE_LAST + 1); - - for (i = 0; i < VIRSH_DOMAIN_HOSTNAME_SOURCE_LAST; i++) - ret[i] = g_strdup(virshDomainHostnameSourceTypeToString(i)); - - return ret; + return virshEnumComplete(VIRSH_DOMAIN_HOSTNAME_SOURCE_LAST, + virshDomainHostnameSourceTypeToString); } @@ -533,20 +512,17 @@ virshDomainPerfEnableCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags) { - size_t i = 0; g_auto(GStrv) events = NULL; const char *event = NULL; virCheckFlags(0, NULL); - events = g_new0(char *, VIR_PERF_EVENT_LAST + 1); - - for (i = 0; i < VIR_PERF_EVENT_LAST; i++) - events[i] = g_strdup(virPerfEventTypeToString(i)); - if (vshCommandOptStringQuiet(ctl, cmd, "enable", &event) < 0) return NULL; + events = virshEnumComplete(VIR_PERF_EVENT_LAST, + virPerfEventTypeToString); + return virshCommaStringListComplete(event, (const char **)events); } @@ -556,20 +532,17 @@ virshDomainPerfDisableCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags) { - size_t i = 0; g_auto(GStrv) events = NULL; const char *event = NULL; virCheckFlags(0, NULL); - events = g_new0(char *, VIR_PERF_EVENT_LAST + 1); - - for (i = 0; i < VIR_PERF_EVENT_LAST; i++) - events[i] = g_strdup(virPerfEventTypeToString(i)); - if (vshCommandOptStringQuiet(ctl, cmd, "disable", &event) < 0) return NULL; + events = virshEnumComplete(VIR_PERF_EVENT_LAST, + virPerfEventTypeToString); + return virshCommaStringListComplete(event, (const char **)events); } @@ -871,19 +844,10 @@ virshDomainSignalCompleter(vshControl *ctl G_GNUC_UNUSED, const vshCmd *cmd G_GNUC_UNUSED, unsigned int flags) { - g_auto(GStrv) tmp = NULL; - size_t i = 0; - virCheckFlags(0, NULL); - tmp = g_new0(char *, VIR_DOMAIN_PROCESS_SIGNAL_LAST + 1); - - for (i = 0; i < VIR_DOMAIN_PROCESS_SIGNAL_LAST; i++) { - const char *name = virshDomainProcessSignalTypeToString(i); - tmp[i] = g_strdup(name); - } - - return g_steal_pointer(&tmp); + return virshEnumComplete(VIR_DOMAIN_PROCESS_SIGNAL_LAST, + virshDomainProcessSignalTypeToString); } @@ -892,19 +856,10 @@ virshDomainLifecycleCompleter(vshControl *ctl G_GNUC_UNUSED, const vshCmd *cmd G_GNUC_UNUSED, unsigned int flags) { - g_auto(GStrv) tmp = NULL; - size_t i = 0; - virCheckFlags(0, NULL); - tmp = g_new0(char *, VIR_DOMAIN_LIFECYCLE_LAST + 1); - - for (i = 0; i < VIR_DOMAIN_LIFECYCLE_LAST; i++) { - const char *name = virshDomainLifecycleTypeToString(i); - tmp[i] = g_strdup(name); - } - - return g_steal_pointer(&tmp); + return virshEnumComplete(VIR_DOMAIN_LIFECYCLE_LAST, + virshDomainLifecycleTypeToString); } @@ -913,19 +868,10 @@ virshDomainLifecycleActionCompleter(vshControl *ctl G_GNUC_UNUSED, const vshCmd *cmd G_GNUC_UNUSED, unsigned int flags) { - g_auto(GStrv) tmp = NULL; - size_t i = 0; - virCheckFlags(0, NULL); - tmp = g_new0(char *, VIR_DOMAIN_LIFECYCLE_ACTION_LAST + 1); - - for (i = 0; i < VIR_DOMAIN_LIFECYCLE_ACTION_LAST; i++) { - const char *action = virshDomainLifecycleActionTypeToString(i); - tmp[i] = g_strdup(action); - } - - return g_steal_pointer(&tmp); + return virshEnumComplete(VIR_DOMAIN_LIFECYCLE_ACTION_LAST, + virshDomainLifecycleActionTypeToString); } @@ -934,19 +880,10 @@ virshCodesetNameCompleter(vshControl *ctl G_GNUC_UNUSED, const vshCmd *cmd G_GNUC_UNUSED, unsigned int flags) { - g_auto(GStrv) tmp = NULL; - size_t i = 0; - virCheckFlags(0, NULL); - tmp = g_new0(char *, VIR_KEYCODE_SET_LAST + 1); - - for (i = 0; i < VIR_KEYCODE_SET_LAST; i++) { - const char *name = virKeycodeSetTypeToString(i); - tmp[i] = g_strdup(name); - } - - return g_steal_pointer(&tmp); + return virshEnumComplete(VIR_KEYCODE_SET_LAST, + virKeycodeSetTypeToString); } @@ -1064,17 +1001,10 @@ virshDomainCoreDumpFormatCompleter(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_DOMAIN_CORE_DUMP_FORMAT_LAST + 1); - - for (i = 0; i < VIR_DOMAIN_CORE_DUMP_FORMAT_LAST; i++) - ret[i] = g_strdup(virshDomainCoreDumpFormatTypeToString(i)); - - return ret; + return virshEnumComplete(VIR_DOMAIN_CORE_DUMP_FORMAT_LAST, + virshDomainCoreDumpFormatTypeToString); } @@ -1100,17 +1030,10 @@ 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; + return virshEnumComplete(VIR_STORAGE_FILE_LAST, + virStorageFileFormatTypeToString); } @@ -1119,17 +1042,10 @@ virshDomainNumatuneModeCompleter(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_DOMAIN_NUMATUNE_MEM_LAST + 1); - - for (i = 0; i < VIR_DOMAIN_NUMATUNE_MEM_LAST; i++) - ret[i] = g_strdup(virDomainNumatuneMemModeTypeToString(i)); - - return ret; + return virshEnumComplete(VIR_DOMAIN_NUMATUNE_MEM_LAST, + virDomainNumatuneMemModeTypeToString); } @@ -1138,15 +1054,8 @@ virshDomainDirtyRateCalcModeCompleter(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 *, VIRSH_DOMAIN_DIRTYRATE_CALC_MODE_LAST + 1); - - for (i = 0; i < VIRSH_DOMAIN_DIRTYRATE_CALC_MODE_LAST; i++) - ret[i] = g_strdup(virshDomainDirtyRateCalcModeTypeToString(i)); - - return ret; + return virshEnumComplete(VIRSH_DOMAIN_DIRTYRATE_CALC_MODE_LAST, + virshDomainDirtyRateCalcModeTypeToString); } diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c index 3795d1fd3a..40cb687582 100644 --- a/tools/virsh-completer-host.c +++ b/tools/virsh-completer-host.c @@ -175,15 +175,8 @@ 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(virshNodeSuspendTargetTypeToString(i)); - - return ret; + return virshEnumComplete(VIR_NODE_SUSPEND_TARGET_LAST, + virshNodeSuspendTargetTypeToString); } diff --git a/tools/virsh-completer-nodedev.c b/tools/virsh-completer-nodedev.c index d10bf2b78c..bf6e809b5a 100644 --- a/tools/virsh-completer-nodedev.c +++ b/tools/virsh-completer-nodedev.c @@ -91,17 +91,14 @@ virshNodeDeviceCapabilityNameCompleter(vshControl *ctl, { g_auto(GStrv) tmp = NULL; const char *cap_str = NULL; - size_t i = 0; virCheckFlags(0, NULL); if (vshCommandOptStringQuiet(ctl, cmd, "cap", &cap_str) < 0) return NULL; - tmp = g_new0(char *, VIR_NODE_DEV_CAP_LAST + 1); - - for (i = 0; i < VIR_NODE_DEV_CAP_LAST; i++) - tmp[i] = g_strdup(virNodeDevCapTypeToString(i)); + tmp = virshEnumComplete(VIR_NODE_DEV_CAP_LAST, + virNodeDevCapTypeToString); return virshCommaStringListComplete(cap_str, (const char **)tmp); } diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c index 84e9d6cc5a..1a966d95b9 100644 --- a/tools/virsh-completer-pool.c +++ b/tools/virsh-completer-pool.c @@ -94,17 +94,14 @@ virshPoolTypeCompleter(vshControl *ctl, { g_auto(GStrv) tmp = NULL; const char *type_str = NULL; - size_t i = 0; virCheckFlags(0, NULL); if (vshCommandOptStringQuiet(ctl, cmd, "type", &type_str) < 0) return NULL; - tmp = g_new0(char *, VIR_STORAGE_POOL_LAST + 1); - - for (i = 0; i < VIR_STORAGE_POOL_LAST; i++) - tmp[i] = g_strdup(virStoragePoolTypeToString(i)); + tmp = virshEnumComplete(VIR_STORAGE_POOL_LAST, + virStoragePoolTypeToString); return virshCommaStringListComplete(type_str, (const char **)tmp); } diff --git a/tools/virsh-completer-volume.c b/tools/virsh-completer-volume.c index bed45fa1ca..a1ebadccac 100644 --- a/tools/virsh-completer-volume.c +++ b/tools/virsh-completer-volume.c @@ -123,15 +123,8 @@ virshStorageVolWipeAlgorithmCompleter(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_VOL_WIPE_ALG_LAST + 1); - - for (i = 0; i < VIR_STORAGE_VOL_WIPE_ALG_LAST; i++) - ret[i] = g_strdup(virshStorageVolWipeAlgorithmTypeToString(i)); - - return ret; + return virshEnumComplete(VIR_STORAGE_VOL_WIPE_ALG_LAST, + virshStorageVolWipeAlgorithmTypeToString); } -- 2.34.1
On Tue, Mar 15, 2022 at 08:08:14AM +0100, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (3): virsh: Properly terminate string list in virshDomainInterfaceSourceModeCompleter() virsh: Introduce virshEnumComplete() virsh: Don't open code virshEnumComplete()
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Michal Privoznik -
Pavel Hrdina