-----Original Message-----
From: Dong, Eddie <eddie.dong@...>
Sent: Saturday, July 23, 2022 2:46 AM
To: acrn-dev@...
Cc: Cao, Minggui <minggui.cao@...>
Subject: RE: [acrn-dev] [PATCH 1/2 v2] hv: shell: improve console to buffer
history cmds

-----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.

[Cao, Minggui] it is a ring buffer, if the buffer is not full filled and full, the index shall be
Handled differently. For example, if total buffer size 8, now just buffered 4, the index shall
Be just in 3-0;

+ 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.

[Cao, Minggui] if the ring buffer has been full, the index could be a full loop,
It need (p_shell->selected_index - 1 + MAX_BUFFERED_CMDS) % MAX_BUFFERED_CMDS
Not same as above.

+
+ if (up_select != p_shell->index_to_store) {

Why not overlap directly? Check is not necessary.

[Cao, Minggui].

+ 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

[Cao, Minggui] fine, will simplify it

+ 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 ?

[Cao, Minggui] yes, will refactor the code.

+ }
+}
+
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?

[Cao, Minggui] it just buffers when current cmd is not same as the last buffered one.
to avoid to waste buffer, and make the buffer list more efficient.

+ 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++.

[Cao, Minggui] fine, will improve.

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.

[Cao, Minggui] yes, right. After refactor, it shall be same.

+ } 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.

[Cao, Minggui] fine, will refactor it.

+ 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