Re: RTEMS | cpukit/shell supports using the tab key to auto-complete commands (!495)
Gedare Bloom (@gedare)
gitlab at rtems.org
Tue Jun 10 17:51:05 UTC 2025
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495 was reviewed by Gedare Bloom
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123338
> +#define RTEMS_SHELL_CMD_SIZE (128)
> +#define RTEMS_SHELL_CMD_COUNT (32)
> +#define RTEMS_SHELL_PROMPT_SIZE (128)
Should these be configurable?
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123339
> +
> +/* Calculate the common prefix length between two strings */
> +static int rtems_shell_calculate_common_prefix(const char *str1, const char *str2, int start_pos, int current_common_len) {
check the format, line lengths >80 should be fixed like https://docs.rtems.org/docs/main/eng/coding-formatting.html#eightycharacterlinelimit (or see formatter pipeline for recommendations)
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123340
> + * Find the boundaries of the current word in a command line
> + */
> +static void rtems_shell_find_word_bounds(char *line, int col, char **word_start, int *word_len) {
I think `col` must be `>=0`, why not use `unsigned`?
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123341
> + }
> +
> + return max_len;
This function could be written a bit differently since you are scanning through str1 and str2, you can (and should) check for the common prefix and the end of each string explicitly. I would suggest:
```
for ( i = start; i < current_common_len; i++ ) {
if (str1[i] == 0 || str2[i] == 0 || str1[i] != str2[i]) {
break;
}
}
return i;
```
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123342
> +}
> +
> +/* Calculate the common prefix length between two strings */
Keep the style consistent with the other functions in this file. Function comments should be like:
```
/*
* Calculate the common prefix length between two strings.
*/
```
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123343
> + }
> +
> + /* Save a copy of original string before moving */
wrong indent level
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123344
> +
> + /* Add original content after completion */
> + strcpy(line + col + chars_to_add, temp);
prefer `strncpy` or `strlcpy`
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123345
> +
> + /* Add completion part */
> + memcpy(line + col, completion + word_len, chars_to_add);
I would prefer to see explicit string terminations. Maybe use `strncpy` here.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123346
> + line[col + chars_to_add + 1] = '\0';
> + chars_to_add++;
> + }
I would prefer explicit string routines, this appears to be something like `strlcat`
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123347
> + chars_to_add++;
> + }
> + }
Is it ok to succeed without writing the suffix?
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123348
> +
> + /* Ensure string is properly terminated */
> + line[size - 1] = '\0';
This can be avoided by using string manipulation routines above.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123349
> + /* Clear current line */
> + fprintf(out, "\r");
> + for (int i = 0; i < size; i++) {
declare `i` earlier
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123350
> + /* Move cursor back to correct position */
> + if (col + chars_to_add < strlen(line)) {
> + for (size_t i = 0; i < strlen(line) - (col + chars_to_add); i++) {
do not redeclare/shadow variable names
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123351
> + int word_len;
> + int matches = 0;
> + char first_match[256];
this is a bit large for on the stack, see below at ln442
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123352
> + if (matches == 0) {
> + /* First match - store it */
> + strncpy(first_match, cmd->name, sizeof(first_match) - 1);
maybe use strdup instead?
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123353
> + * Find the boundaries of the current word in a command line
> + */
> +static void rtems_shell_find_word_bounds(char *line, int col, char **word_start, int *word_len) {
instead of returning `word_len` by reference, you could return it directly as the return value
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123354
> + * Find the boundaries of the current word in a command line
> + */
> +static void rtems_shell_find_word_bounds(char *line, int col, char **word_start, int *word_len) {
i think word_start could just be a string (`char *`)? You don't need the reference to the original string itself. I would pass it as `const char *`
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123355
> +
> + /* Add original content after completion */
> + strcpy(line + col + chars_to_add, temp);
Instead of this way of copying the string twice, you can do this more efficiently in place, something like:
```
strlcpy(line+col+chars_to_add, line+col, chars_to_add+1);
strncpy(line+col, completion+ word_len, chars_to_add);
```
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123356
> + strncpy(first_match, cmd->name, sizeof(first_match) - 1);
> + first_match[sizeof(first_match) - 1] = '\0';
> + common_len = strlen(cmd->name);
This is incorrect in case `first_match` is smaller than `cmd->name`.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123357
> + cmd = rtems_shell_first_cmd;
> + while (cmd) {
> + if (strncmp(cmd->name, partial_cmd, word_len) == 0) {
Do you need `partial_cmd` here, or can you use `word_start` directly?
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123358
> + }
> + cmd = cmd->next;
> + }
I would refactor this into a helper function for searching and printing the set of possible commands for completion.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123359
> +
> + /* Complete as much as possible */
> + int chars_added = 0;
declare all variables at the start of their scope/block.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123360
> +/* Insert completion content into command line */
> +static int insert_completion(char *line, int col, int size, FILE *out,
> + const char *completion, int word_len, int common_len,
This seems too many parameters, try to simplify/refactor this function. Even with modern calling conventions, it becomes inefficient to have more than about 4-6 arguments.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123361
> + struct dirent *entry;
> + int matches = 0;
> + char first_match[256];
same as above, avoid allocating large buffers on the stack.
This is especially problematic when doing string manipulations. Getting this correct without introducing stack buffer overflow is a challenge.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123362
> + filename = partial_name;
> + }
> + }
Path handling code in C is challenging to get correct and secure. I'm not convinced this is the right approach to take.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123363
> + filename = last_slash + 1;
> + } else {
> + dir_path = ".";
There is no slash at the end of this, is it OK?
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123364
> + matches++;
> + }
> + }
please refactor long chunks of logic to make simpler functions.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123365
> + struct stat st;
> +
> + char full_path[512] = {0};// Using 512 instead of PATH_MAX as PATH_MAX is too large
avoid large buffers on stack.
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123366
> int inserting = 1;
> int in_fileno = fileno(in);
> + int chars_added = 0;
match formatting style to surrounding code
--
Gedare Bloom started a new discussion on cpukit/libmisc/shell/shell.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495#note_123367
> + fprintf(out, "\r%s%s", prompt, line);
> + /* Position cursor correctly */
> + int back_chars = strlen(line) - col;
declare variable at start of block.
--
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/495
You're receiving this email because of your account on gitlab.rtems.org.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20250610/7edaf53f/attachment-0001.htm>
More information about the bugs
mailing list