[v1,2/2] app/testpmd: sort commands by help string
Checks
Commit Message
When using '?' to find commands, it occasionally is difficult to find the
needed commands because all commands are not in alphabetical order, but
rather can be ordered rather arbitrarily.
To address this, use help string to order commands. This sacrifices some
amount of grouping (i.e. when tm commands go one after another), but may
improve discoverability (and most similar commands tend to have similar
help strings and will be located closer together anyway).
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
app/test-pmd/cmdline.c | 82 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
Comments
On Wed, May 14, 2025 at 02:02:12PM +0100, Anatoly Burakov wrote:
> When using '?' to find commands, it occasionally is difficult to find the
> needed commands because all commands are not in alphabetical order, but
> rather can be ordered rather arbitrarily.
>
> To address this, use help string to order commands. This sacrifices some
> amount of grouping (i.e. when tm commands go one after another), but may
> improve discoverability (and most similar commands tend to have similar
> help strings and will be located closer together anyway).
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> app/test-pmd/cmdline.c | 82 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
>
+1 to sorting
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
On Wed, 14 May 2025 14:02:12 +0100
Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> When using '?' to find commands, it occasionally is difficult to find the
> needed commands because all commands are not in alphabetical order, but
> rather can be ordered rather arbitrarily.
>
> To address this, use help string to order commands. This sacrifices some
> amount of grouping (i.e. when tm commands go one after another), but may
> improve discoverability (and most similar commands tend to have similar
> help strings and will be located closer together anyway).
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Sorting is good, but why not pre-sort builtin_ctx and make it const?
On 5/22/2025 12:24 AM, Stephen Hemminger wrote:
> On Wed, 14 May 2025 14:02:12 +0100
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
>
>> When using '?' to find commands, it occasionally is difficult to find the
>> needed commands because all commands are not in alphabetical order, but
>> rather can be ordered rather arbitrarily.
>>
>> To address this, use help string to order commands. This sacrifices some
>> amount of grouping (i.e. when tm commands go one after another), but may
>> improve discoverability (and most similar commands tend to have similar
>> help strings and will be located closer together anyway).
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>
> Sorting is good, but why not pre-sort builtin_ctx and make it const?
>
It's difficult and error-prone to sort these messages statically because
there's no obvious to know where to insert the new value without reading
lots of help strings from other commands. You could argue that we could
sort by variable name but it feels like an unnecessary burden on the
developer to maintain this sorting order when we can just automate it at
runtime without giving up anything.
@@ -13951,6 +13951,78 @@ testpmd_add_driver_commands(struct testpmd_driver_commands *c)
TAILQ_INSERT_TAIL(&driver_commands_head, c, next);
}
+static int
+cmd_cmp(const void *l, const void *r)
+{
+ cmdline_parse_inst_t *l_cmd = *(cmdline_parse_inst_t * const*)l;
+ cmdline_parse_inst_t *r_cmd = *(cmdline_parse_inst_t * const*)r;
+ int l_idx = -1, r_idx = -1;
+ size_t i;
+
+ /* these commands should always be at the top of the list */
+ cmdline_parse_inst_t const *top_cmds[] = {
+ &cmd_help_brief,
+ &cmd_help_long,
+ &cmd_quit,
+ &cmd_load_from_file,
+ &cmd_start,
+ &cmd_stop,
+ };
+
+ /* there may be NULL values - push them to the end */
+ if (l_cmd == NULL)
+ return 1;
+ if (r_cmd == NULL)
+ return -1;
+
+ /* should circuit on equality - shouldn't happen anyway */
+ if (l_cmd == r_cmd)
+ return 0;
+
+ /* try finding both commands in list header */
+ for (i = 0; i < RTE_DIM(top_cmds); i++) {
+ cmdline_parse_inst_t const *tmp = top_cmds[i];
+ if (l_idx == -1 && l_cmd == tmp)
+ l_idx = i;
+ if (r_idx == -1 && r_cmd == tmp)
+ r_idx = i;
+ }
+
+ if (l_idx >= 0 && r_idx < 0)
+ /* l in list, r not in list */
+ return -1;
+ if (l_idx < 0 && r_idx >= 0)
+ /* l not in list, r in list */
+ return 1;
+ if (l_idx >= 0 && r_idx >= 0)
+ /* both in list */
+ return l_idx < r_idx ? -1 : 1;
+
+ /*
+ * Neither are in top list. Comparing commands is difficult because
+ * there are multiple types of tokens, some of which are custom and
+ * cannot be generalized. What we'll do instead is compare help strings,
+ * because those tend to be fairly consistently specified.
+ */
+ const char *l_helpstr = l_cmd->help_str;
+ const char *r_helpstr = r_cmd->help_str;
+ int ret;
+
+ /* some commands do not have help strings, push them to the back */
+ if (l_helpstr == NULL && r_helpstr == NULL)
+ return 0;
+ if (l_helpstr == NULL)
+ return 1;
+ if (r_helpstr == NULL)
+ return -1;
+
+ /* we need case-insensitive compare */
+ ret = strcmp(l_helpstr, r_helpstr);
+ if (ret == 0)
+ return 0;
+ return ret < 0 ? -1 : 1;
+}
+
int
init_cmdline(void)
{
@@ -13962,6 +14034,10 @@ init_cmdline(void)
cmd_set_fwd_mode_init();
cmd_set_fwd_retry_mode_init();
+ /* sort built-in command array */
+ qsort(builtin_ctx, RTE_DIM(builtin_ctx) - 1, sizeof(builtin_ctx[0]), cmd_cmp);
+
+ /* count total number of commands */
count = 0;
for (i = 0; builtin_ctx[i] != NULL; i++)
count++;
@@ -13975,12 +14051,18 @@ init_cmdline(void)
if (main_ctx == NULL)
return -1;
+ /* copy built-in commands */
count = 0;
for (i = 0; builtin_ctx[i] != NULL; i++, count++)
main_ctx[count] = builtin_ctx[i];
+
+ /* copy driver-specific commands */
TAILQ_FOREACH(c, &driver_commands_head, next) {
+ size_t start = count;
for (i = 0; c->commands[i].ctx != NULL; i++, count++)
main_ctx[count] = c->commands[i].ctx;
+ /* sort driver-specific commands, per driver */
+ qsort(&main_ctx[start], i, sizeof(main_ctx[0]), cmd_cmp);
}
return 0;