Re: [PATCH 1/2 v2] hv: shell: improve console to buffer history cmds


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Minggui Cao
Sent: Thursday, July 21, 2022 10:56 PM
To: acrn-dev@...
Cc: Cao, Minggui <minggui.cao@...>
Subject: [acrn-dev] [PATCH 1/2 v2] hv: shell: improve console to buffer history
cmds

1. buffer max to 8 history commands.
2. support up/down key to select history buffered commands

Signed-off-by: Minggui Cao <minggui.cao@...>
---
hypervisor/debug/shell.c | 140 ++++++++++++++++++++++++++++------
hypervisor/debug/shell_priv.h | 9 +++
2 files changed, 127 insertions(+), 22 deletions(-)

diff --git a/hypervisor/debug/shell.c b/hypervisor/debug/shell.c index
588e4aa0f..0bc982c27 100644
--- a/hypervisor/debug/shell.c
+++ b/hypervisor/debug/shell.c
@@ -158,6 +158,14 @@ static struct shell_cmd shell_cmds[] = {
},
};

+/* for function key: up/down key */
+enum function_key {
+ KEY_NONE,
+
+ KEY_UP = 0x5B41,
+ KEY_DOWN = 0x5B42,
+};
+
/* The initial log level*/
uint16_t console_loglevel = CONFIG_CONSOLE_LOGLEVEL_DEFAULT; uint16_t
mem_loglevel = CONFIG_MEM_LOGLEVEL_DEFAULT; @@ -271,14 +279,89
@@ static uint16_t sanitize_vmid(uint16_t vmid)
return sanitized_vmid;
}

+static void clear_input_line(uint32_t len) {
+ while (len > 0) {
+ len--;
+ shell_puts("\b");
+ shell_puts(" \b");
+ }
+}
+static void copy_chars(char *dest, char *src, uint32_t size, bool
+from_head) {
+ uint32_t i = 0;
+
+ if (from_head) {
+ for (i = 0; i < size; i++) {
+ *(dest + i) = *(src + i);
+ }
+ } else {
+ for (i = size; i > 0; i--) {
+ *(dest + i - 1) = *(src + i - 1);
+ }
+ }
+}
+
+static void handle_updown_key(enum function_key key_value) {
+ uint32_t current_select = p_shell->selected_index;
+
+ if (p_shell->total_buffered_cmds == 0) {
+ return;
+ }
+
+ /* update p_shell->selected_index as up/down key */
+ if (key_value == KEY_UP) {
+
+ if ((p_shell->total_buffered_cmds < MAX_BUFFERED_CMDS)
&& (p_shell->selected_index > 0)) {
Didn't understand why we need to check "p_shell->total_buffered_cmds < MAX_BUFFERED_CMDS" here.

+ p_shell->selected_index--;
.....

+ } else if (p_shell->total_buffered_cmds >=
MAX_BUFFERED_CMDS) {
+ uint32_t up_select = (p_shell->selected_index +
MAX_BUFFERED_CMDS -
+1) % MAX_BUFFERED_CMDS;
Don't understand why we need this conditional process. I think they can be same.


+
+ if (up_select != p_shell->index_to_store) {
Why not overlap directly? Check is not necessary.

+ p_shell->selected_index = up_select;
+ }
+ }
+ } else { /* for down-key */
+ uint32_t down_select = (p_shell->selected_index + 1) %
+MAX_BUFFERED_CMDS;
+
+ if (down_select != p_shell->index_to_store) {
Ditto

+ p_shell->selected_index = down_select;
+ }
+ }
+
+ if (strcmp(p_shell->buffered_cmds[current_select],
+p_shell->input_line[p_shell->input_line_active]) != 0) {
+
+ clear_input_line(p_shell->input_line_len);
+ shell_puts(p_shell->buffered_cmds[current_select]);
+
+ size_t len = strnlen_s(p_shell->buffered_cmds[current_select],
+SHELL_CMD_MAX_LEN);
+
+ copy_chars(p_shell->input_line[p_shell->input_line_active],
+ p_shell->buffered_cmds[current_select], len + 1,
true);
+
+ p_shell->input_line_len = len;
Load a buffed line, right?
A separate wrapper ?

+ }
+}
+
static void shell_handle_special_char(char ch) {
+ enum function_key key_value = KEY_NONE;
+
switch (ch) {
- /* Escape character */
+ /* original function key value: ESC + key, so consume the next 2
+characters */
case 0x1b:
- /* Consume the next 2 characters */
- (void) shell_getc();
- (void) shell_getc();
+ key_value = (shell_getc() << 8) | shell_getc();
+
+ switch (key_value) {
+ case KEY_UP:
+ case KEY_DOWN:
+ handle_updown_key(key_value);
+ break;
+ default:
+ break;
+ }
+
break;
default:
/*
@@ -427,31 +510,40 @@ static int32_t shell_process(void) {
int32_t status;
char *p_input_line;
+ bool to_buffer = true;

- /* Check for the repeat command character in active input line.
- */
- if (p_shell->input_line[p_shell->input_line_active][0] == '.') {
- /* Repeat the last command (using inactive input line).
- */
- p_input_line =
- &p_shell->input_line[SHELL_INPUT_LINE_OTHER
- (p_shell->input_line_active)][0];
- } else {
- /* Process current command (using active input line). */
- p_input_line =
- &p_shell->input_line[p_shell->input_line_active][0];
+ /* Process current command (using active input line). */
+ p_input_line = p_shell->input_line[p_shell->input_line_active];

- /* Switch active input line. */
- p_shell->input_line_active =
- SHELL_INPUT_LINE_OTHER(p_shell-
input_line_active);
- }
+ /* Switch active input line. */
+ p_shell->input_line_active =
+SHELL_INPUT_LINE_OTHER(p_shell->input_line_active);

/* Process command */
status = shell_process_cmd(p_input_line);

+ /* buffer the handled cmd */
+ if (p_shell->total_buffered_cmds > 0) {
+ uint32_t former_index = (p_shell->index_to_store +
MAX_BUFFERED_CMDS
+- 1) % MAX_BUFFERED_CMDS;
+
+ if (strcmp(p_input_line, p_shell-
buffered_cmds[former_index]) == 0) {
Seems redundant check. What happens if we always overlap the buffered line?


+ to_buffer = false;
+ }
+ }
+
+ if (to_buffer && (strnlen_s(p_input_line, SHELL_CMD_MAX_LEN) > 0))
{
+ strncpy_s(p_shell->buffered_cmds[p_shell->index_to_store],
SHELL_CMD_MAX_LEN,
+ p_input_line, SHELL_CMD_MAX_LEN);
+
+ p_shell->selected_index = p_shell->index_to_store;
+ p_shell->index_to_store = (p_shell->index_to_store + 1) %
MAX_BUFFERED_CMDS;
+ p_shell->total_buffered_cmds = (p_shell-
total_buffered_cmds >= MAX_BUFFERED_CMDS) ?
+ MAX_BUFFERED_CMDS : (p_shell-
total_buffered_cmds + 1);
Prefer:
if (...)
p_shell->total_buffered_cmds++.

More clear.

But from the implementation, So we only buffer the lines with ENTER.
I am fine, but bash can buffer temporary edit buffer too.


+ } else {
+ p_shell->selected_index = (p_shell->index_to_store +
MAX_BUFFERED_CMDS - 1) % MAX_BUFFERED_CMDS;
+ }
+
/* Now that the command is processed, zero fill the input buffer */
- (void)memset((void *) p_shell->input_line[p_shell->input_line_active],
- 0, SHELL_CMD_MAX_LEN + 1U);
+ (void)memset((void *) p_shell->input_line[p_shell->input_line_active],
+0, SHELL_CMD_MAX_LEN + 1U);

/* Process command and return result to caller */
return status;
@@ -490,6 +582,10 @@ void shell_init(void)
p_shell->cmds = shell_cmds;
p_shell->cmd_count = ARRAY_SIZE(shell_cmds);

+ p_shell->index_to_store = 0;
+ p_shell->total_buffered_cmds = 0;
+ p_shell->selected_index = 0;
+
/* Zero fill the input buffer */
(void)memset((void *)p_shell->input_line[p_shell->input_line_active],
0U,
SHELL_CMD_MAX_LEN + 1U);
diff --git a/hypervisor/debug/shell_priv.h b/hypervisor/debug/shell_priv.h index
addad6555..ce40f3cd0 100644
--- a/hypervisor/debug/shell_priv.h
+++ b/hypervisor/debug/shell_priv.h
@@ -26,11 +26,20 @@ struct shell_cmd {

};

+#define MAX_BUFFERED_CMDS 8
+
/* Shell Control Block */
struct shell {
char input_line[2][SHELL_CMD_MAX_LEN + 1U]; /* current & last */
uint32_t input_line_len; /* Length of current input line */
uint32_t input_line_active; /* Active input line index */
+
+ /* a ring buffer to buffer former commands */
+ char buffered_cmds[MAX_BUFFERED_CMDS][SHELL_CMD_MAX_LEN
+ 1U];
With this extension, the concept of current and last input_line is no longer valid.
Why we not extend input_line[] to be buffered_line?

p_shell->input_line_active can be the current buffer/line.



+ uint32_t total_buffered_cmds;
While.


+ uint32_t index_to_store
Add comment:
/* Index of current buffered_cmds to store the CMD */

+ uint32_t selected_index; /* used for up/down key to select former
cmds
+*/
/* Index of selected buffered_cmds chosen by up/down key */


+
struct shell_cmd *cmds; /* cmds supported */
uint32_t cmd_count; /* Count of cmds supported */
};
--
2.25.1




Join acrn-dev@lists.projectacrn.org to automatically receive all group messages.