LU-16392 utils: use --list-commands for bash completion

The CLI utils lctl and lfs currently use a pseudo option
--non-existent-option to generate a list of completions. However, this
was broken when the help output for an invalid command was changed.
Using --list-commands instead means that the format of the help output
can be kept succinct.

However, currently there are 2 issues that make --list-commands
unsuitable.

First, --list-commands truncates long commands. This commit resolves
this by not truncating long commands, and removing the fixed-length
char buffer and writing directly to stdout so that the line length
can overflow slightly if needed.

Second, --list-commands recursively displays sub-commands. For
example, for `lctl`, it will display `pcc add`, `pcc del`, etc in
additon to just `pcc`. The bash completion tools would view these
as separate tokens and thus would inappropriately suggest `add`,
`del`, etc. as completions for `lctl`. This commit removes the
recursive behavior.

Removing the recursive behavior resolves an unrelated bug with the
recursion that can be observed for `lctl`, where a number of
top-level commands are skipped following recursion into a previous
sub-command, equal to the number of subcommands processed in the
recursive call. Specifically, the commands in the section "device
setup", e.g. `attach`, `detach`, were not displayed following the
recursive call into `pcc`.

Finally, this commit changes the command parser to recognize --help
and print the list of commands when this argument is seen.

Lustre-change: https://review.whamcloud.com/49484
Lustre-commit: b4cc570ad11c1c07a6e1d825787ccc62c1245ca1

Fixes: bc69a8d058 ("LU-8621 utils: cmd help to stdout or short cmd error")
Signed-off-by: Thomas Bertschinger <bertschinger@lanl.gov>
Change-Id: Ib6e139402b9cd18e5a54b8fd3d6a2652d301e736
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Vitaliy Kuznetsov <vkuznetsov@ddn.com>
Reviewed-on: https://review.whamcloud.com/c/ex/lustre-release/+/53337
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Alex Deiter <alex.deiter@gmail.com>

diff --git a/libcfs/include/libcfs/util/parser.h b/libcfs/include/libcfs/util/parser.h
index 7bae839..fc9d42d 100644 (file)
--- a/libcfs/include/libcfs/util/parser.h
+++ b/libcfs/include/libcfs/util/parser.h
@@ -76,10 +76,8 @@ void Parser_ignore_errors(int ignore);       /* Set the ignore errors flag */
 void Parser_printhelp(char *);         /* Detailed help routine */
 void Parser_exit(int, char **);                /* Shuts down command parser */
 int Parser_execarg(int argc, char **argv, command_t cmds[]);
-int execute_line(char * line);
-int Parser_list_commands(const command_t *cmdlist, char *buffer,
-                        size_t buf_size, const char *parent_cmd,
-                        int col_start, int col_num);
+int Parser_list_commands(const command_t *cmdlist, int line_len,
+                        int col_num);
 
 /* Converts a string to an integer */
 int Parser_int(char *, int *);

diff --git a/libcfs/libcfs/util/parser.c b/libcfs/libcfs/util/parser.c
index c5c8947..4e4b0cd 100644 (file)
--- a/libcfs/libcfs/util/parser.c
+++ b/libcfs/libcfs/util/parser.c
@@ -121,10 +121,21 @@ int Parser_execarg(int argc, char **argv, command_t cmds[])
                        fprintf(stderr, "%s\n", cmd->pc_help);
                return rc;
        }
-       printf("Try interactive use without arguments or use one of:\n");
-       for (cmd = cmds; cmd->pc_name; cmd++)
-               printf("\"%s\"\n", cmd->pc_name);
-       printf("as argument.\n");
+       /*
+        * handle --help here as a synonym of --list-commands so that it can be
+        * applied to all commands without needing to individually add a --help
+        * option to each command.
+        * This does not apply to lctl which has its own --help option because
+        * `lctl --list-commands` uses a unique format.
+        */
+       if (strcmp(argv[0], "--help") == 0) {
+               Parser_list_commands(cmds, 80, 4);
+               return 0;
+       }
+       fprintf(stderr,
+               "%s: '%s' is not a valid command. See '%s --list-commands'.\n",
+               program_invocation_short_name, argv[0],
+               program_invocation_short_name);
 
        return -1;
 }
@@ -539,72 +550,49 @@ void Parser_printhelp(char *cmd)
 /**
  * Parser_list_commands() - Output a list of the supported commands.
  * @cmdlist:     Array of structures describing the commands.
- * @buffer:      String buffer used to temporarily store the output text.
- * @buf_size:    Length of the string buffer.
- * @parent_cmd:          When called recursively, contains the name of the parent cmd.
- * @col_start:   Column where printing should begin.
+ * @line_len:    Length of output line.
  * @col_num:     The number of commands printed in a single row.
  *
  * The commands and subcommands supported by the utility are printed, arranged
- * into several columns for readability.  If a command supports subcommands, the
- * function is called recursively, and the name of the parent command is
- * supplied so that it can be prepended to the names of the subcommands.
+ * into several columns for readability.
  *
  * Return: The number of items that were printed.
  */
-int Parser_list_commands(const command_t *cmdlist, char *buffer,
-                        size_t buf_size, const char *parent_cmd,
-                        int col_start, int col_num)
+int Parser_list_commands(const command_t *cmdlist, int line_len,
+                        int col_num)
 {
-       int col = col_start;
        int char_max;
-       int len;
        int count = 0;
-       int rc;
+       int col = 0;
 
-       if (col_start >= col_num)
-               return 0;
+       int nprinted = 0;
+       int offset = 0;
 
-       char_max = (buf_size - 1) / col_num; /* Reserve 1 char for NUL */
+       char_max = line_len / col_num;
 
        for (; cmdlist->pc_name; cmdlist++) {
                if (!cmdlist->pc_func && !cmdlist->pc_sub_cmd)
                        break;
                count++;
-               if (parent_cmd)
-                       len = snprintf(&buffer[col * char_max],
-                                      char_max + 1, "%s %s", parent_cmd,
-                                      cmdlist->pc_name);
-               else
-                       len = snprintf(&buffer[col * char_max],
-                                      char_max + 1, "%s", cmdlist->pc_name);
-
-               /* Add trailing spaces to pad the entry to the column size */
-               if (len < char_max) {
-                       snprintf(&buffer[col * char_max] + len,
-                                char_max - len + 1, "%*s", char_max - len,
-                                " ");
-               } else {
-                       buffer[(col + 1) * char_max - 1] = ' ';
-               }
+
+               nprinted = fprintf(stdout, "%-*s ", char_max - offset - 1,
+                                  cmdlist->pc_name);
+               /*
+                * when a column is too wide, save offset so subsequent columns
+                * can be aligned properly
+                */
+               offset = offset + nprinted - char_max;
+               offset = offset > 0 ? offset : 0;
 
                col++;
                if (col >= col_num) {
-                       fprintf(stdout, "%s\n", buffer);
+                       fprintf(stdout, "\n");
                        col = 0;
-                       buffer[0] = '\0';
-               }
-
-               if (cmdlist->pc_sub_cmd) {
-                       rc = Parser_list_commands(cmdlist->pc_sub_cmd, buffer,
-                                                 buf_size, cmdlist->pc_name,
-                                                 col, col_num);
-                       col = (col + rc) % col_num;
-                       count += rc;
+                       offset = 0;
                }
        }
-       if (!parent_cmd && col != 0)
-               fprintf(stdout, "%s\n", buffer);
+       if (col != 0)
+               fprintf(stdout, "\n");
        return count;
 }
 

diff --git a/lnet/utils/lnetctl.c b/lnet/utils/lnetctl.c
index 720c4fe..2528db0 100644 (file)
--- a/lnet/utils/lnetctl.c
+++ b/lnet/utils/lnetctl.c
@@ -2295,9 +2295,7 @@ static int jt_discover(int argc, char **argv)
 
 static int lnetctl_list_commands(int argc, char **argv)
 {
-       char buffer[81] = ""; /* 80 printable chars + terminating NUL */
-
-       Parser_list_commands(cmd_list, buffer, sizeof(buffer), NULL, 0, 4);
+       Parser_list_commands(cmd_list, 80, 4);
 
        return 0;
 }

diff --git a/lnet/utils/lst.c b/lnet/utils/lst.c
index 3509137..3ea8834 100644 (file)
--- a/lnet/utils/lst.c
+++ b/lnet/utils/lst.c
@@ -3289,9 +3289,7 @@ lst_initialize(void)
 
 static int lst_list_commands(int argc, char **argv)
 {
-       char buffer[81] = ""; /* 80 printable chars + terminating NUL */
-
-       Parser_list_commands(lst_cmdlist, buffer, sizeof(buffer), NULL, 0, 4);
+       Parser_list_commands(lst_cmdlist, 80, 4);
 
        return 0;
 }

diff --git a/lustre/scripts/bash-completion/lustre b/lustre/scripts/bash-completion/lustre
index 7e5c31c..51afc07 100644 (file)
--- a/lustre/scripts/bash-completion/lustre
+++ b/lustre/scripts/bash-completion/lustre
@@ -3,9 +3,8 @@ _lustre_cmds()
        local cmd="$1"
        local sub="$2"
 
-       # "--list-command" prints commands in columns and truncates long ones
-       "$cmd" "$sub" --non-existent-option |
-               sed -e 1d -e '$d' -e 's/"//g' -e /=/d -e /exit/d -e /quit/d
+       $cmd $sub --list-commands |
+               sed -e '/=/d' -e s/exit// -e s/quit//
 }
 
 _lustre_long_opts()

diff --git a/lustre/tests/sanity.sh b/lustre/tests/sanity.sh
index be1f811..b066f1a 100755 (executable)
--- a/lustre/tests/sanity.sh
+++ b/lustre/tests/sanity.sh
@@ -28087,6 +28087,30 @@ test_434() {
 }
 run_test 434 "Client should not send RPCs for security.selinux with SElinux disabled"
 
+test_440() {
+       if [[ -f $LUSTRE/scripts/bash-completion/lustre ]]; then
+               source $LUSTRE/scripts/bash-completion/lustre
+       elif [[ -f /usr/share/bash-completion/completions/lustre ]]; then
+               source /usr/share/bash-completion/completions/lustre
+       else
+               skip "bash completion scripts not found"
+       fi
+
+       local lctl_completions
+       local lfs_completions
+
+       lctl_completions=$(_lustre_cmds lctl)
+       if [[ ! $lctl_completions =~ "get_param" ]]; then
+               error "lctl bash completion failed"
+       fi
+
+       lfs_completions=$(_lustre_cmds lfs)
+       if [[ ! $lfs_completions =~ "setstripe" ]]; then
+               error "lfs bash completion failed"
+       fi
+}
+run_test 440 "bash completion for lfs, lctl"
+
 test_450() {
        remote_ost_nodsh && skip "remote OST with nodsh" && return
        local mntdev

diff --git a/lustre/utils/lctl.c b/lustre/utils/lctl.c
index c88288e..01c50b6 100644 (file)
--- a/lustre/utils/lctl.c
+++ b/lustre/utils/lctl.c
@@ -77,7 +77,7 @@ command_t pccdev_cmdlist[] = {
        { .pc_name = "list", .pc_func = jt_pcc_list,
          .pc_help = "List all PCC backends on a client.\n"
                "usage: lctl pcc list <mntpath>\n" },
-       { .pc_name = "list-commands", .pc_func = jt_pcc_list_commands,
+       { .pc_name = "--list-commands", .pc_func = jt_pcc_list_commands,
          .pc_help = "list commands supported by lctl pcc"},
        { .pc_name = "help", .pc_func = Parser_help, .pc_help = "help" },
        { .pc_name = "exit", .pc_func = Parser_quit, .pc_help = "quit" },
@@ -110,6 +110,9 @@ command_t cmdlist[] = {
         "print build version of this utility and exit"},
        {"--list-commands", lctl_list_commands, 0,
         "list commands supported by this utility and exit"},
+       /* help is a synonym of --list-commands */
+       {"--help", lctl_list_commands, 0,
+        "list commands supported by this utility and exit"},
 
        /* Network configuration commands */
        {"===== network config =====", NULL, 0, "network config"},
@@ -603,10 +606,7 @@ command_t cmdlist[] = {
  */
 static int jt_pcc_list_commands(int argc, char **argv)
 {
-       char buffer[81] = "";
-
-       Parser_list_commands(pccdev_cmdlist, buffer, sizeof(buffer),
-                            NULL, 0, 4);
+       Parser_list_commands(pccdev_cmdlist, 80, 4);
 
        return 0;
 }
@@ -670,7 +670,6 @@ int lctl_main(int argc, char **argv)
 
 static int lctl_list_commands(int argc, char **argv)
 {
-       char buffer[81] = ""; /* 80 printable chars + terminating NUL */
        command_t *cmd;
        int rc;
 
@@ -678,8 +677,7 @@ static int lctl_list_commands(int argc, char **argv)
        while (cmd->pc_name != NULL) {
                printf("\n%s\n", cmd->pc_name); /* Command category */
                cmd++;
-               rc = Parser_list_commands(cmd, buffer, sizeof(buffer), NULL,
-                                        0, 4);
+               rc = Parser_list_commands(cmd, 80, 4);
                cmd += rc;
        }
 

diff --git a/lustre/utils/lfs.c b/lustre/utils/lfs.c
index fd8e9e1..d211034 100644 (file)
--- a/lustre/utils/lfs.c
+++ b/lustre/utils/lfs.c
@@ -384,10 +384,9 @@ command_t mirror_cmdlist[] = {
                "<mirrored file> [<mirrored file2>...]\n"},
        { .pc_name = "verify", .pc_func = lfs_mirror_verify,
          .pc_help = "Verify mirrored file(s).\n"
-               "usage: lfs mirror verify "
-               "[--only <mirror_id,mirror_id2[,...]>] "
-               "[--verbose|-v] <mirrored_file> [<mirrored_file2> ...]\n"},
-       { .pc_name = "list-commands", .pc_func = lfs_mirror_list_commands,
+               "usage: lfs mirror verify [--only MIRROR_ID[,...]]\n"
+               "\t\t[--verbose|-v] <mirrored_file> [<mirrored_file2> ...]\n" },
+       { .pc_name = "--list-commands", .pc_func = lfs_mirror_list_commands,
          .pc_help = "list commands supported by lfs mirror"},
        { .pc_name = "help", .pc_func = Parser_help, .pc_help = "help" },
        { .pc_name = "exit", .pc_func = Parser_quit, .pc_help = "quit" },
@@ -439,7 +438,7 @@ command_t pcc_cmdlist[] = {
        { .pc_name = "unpin", .pc_func = lfs_pcc_unpin,
          .pc_help = "Un-pin files so that they can be removed from PCC by lpcc_purge.\n"
                "usage: lfs pcc unpin [--id|-i ID] FILE ...\n"},
-       { .pc_name = "list-commands", .pc_func = lfs_pcc_list_commands,
+       { .pc_name = "--list-commands", .pc_func = lfs_pcc_list_commands,
          .pc_help = "list commands supported by lfs pcc"},
        { .pc_name = "help", .pc_func = Parser_help, .pc_help = "help" },
        { .pc_name = "exit", .pc_func = Parser_quit, .pc_help = "quit" },
@@ -13015,10 +13014,7 @@ static int lfs_somsync(int argc, char **argv)
  */
 static int lfs_mirror_list_commands(int argc, char **argv)
 {
-       char buffer[81] = "";
-
-       Parser_list_commands(mirror_cmdlist, buffer, sizeof(buffer),
-                            NULL, 0, 4);
+       Parser_list_commands(mirror_cmdlist, 80, 4);
 
        return 0;
 }
@@ -13702,10 +13698,7 @@ static int lfs_pcc_unpin(int argc, char **argv)
  */
 static int lfs_pcc_list_commands(int argc, char **argv)
 {
-       char buffer[81] = "";
-
-       Parser_list_commands(pcc_cmdlist, buffer, sizeof(buffer),
-                            NULL, 0, 4);
+       Parser_list_commands(pcc_cmdlist, 80, 4);
 
        return 0;
 }
@@ -13742,9 +13735,7 @@ static int lfs_pcc(int argc, char **argv)
 
 static int lfs_list_commands(int argc, char **argv)
 {
-       char buffer[81] = ""; /* 80 printable chars + terminating NUL */
-
-       Parser_list_commands(cmdlist, buffer, sizeof(buffer), NULL, 0, 4);
+       Parser_list_commands(cmdlist, 80, 4);
 
        return 0;
 }