-----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)) {

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

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

+ 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) {

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

+ }
+}
+
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) {

+ 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);

+ } 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];

+ uint32_t total_buffered_cmds;

+ uint32_t index_to_store

+ uint32_t selected_index; /* used for up/down key to select former
cmds
+*/

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

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

+ 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

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


Hi, Eddie,
Thanks for your comments!
Before the feature as follows:
1. it is a ring buffer, just store the new one if it is not same with last
one in buffer.

2. just buffer the command after "enter" key

I need re-consider how to refactor this feature after checking your
comments:
1. could combine input_line with buffered_cmds as one array.

For other comments, see inline answers.

Thanks
Minggui

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

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

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


Hi, Eddie,
Thanks for your comments!
Before the feature as follows:
1. it is a ring buffer, just store the new one if it is not same with
last one in buffer.

OK

2. just buffer the command after "enter" key

Fine, though it is different with bash.

I need re-consider how to refactor this feature after checking
your
comments:
1. could combine input_line with buffered_cmds as one array.

For other comments, see inline answers.

Thanks
Minggui

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

Understood. But we don't need to check it.

For key_UP, it can do like:
If ( --p_shell->selected_index < 0 )
p_shell->selected_index += p_shell->total_buffered_cmds;

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

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

[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;

Understood. But we don't need to check it.

For key_UP, it can do like:
If ( --p_shell->selected_index < 0 )
p_shell->selected_index += p_shell->total_buffered_cmds;

[Cao, Minggui] there is a rule for history cmds: it is not a loop, if
press up or down key always, it will go up/down to the first or last
command as the end one, so the user will know the history is to end.
1. if the ring buffer not full, up key will go until 0 index, then
it will not change any more.
2. if the ring buffer is full, up key will also go to an end index
too and will not change any more then.

Why we need this kind of difference? I am not convinced/

I updated this patch to V3, and sent it out, combine "input_line" and
"buffered_cmds" as "buffered_line", and simplify some logic as your

comments.

Could you have a review?

-----Original Message-----
From: Cao, Minggui <minggui.cao@...>
Sent: Monday, July 25, 2022 8:12 PM
To: Dong, Eddie <eddie.dong@...>; acrn-dev@...
Subject: RE: [acrn-dev] [PATCH 1/2 v2] hv: shell: improve console to buffer
history cmds

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

[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;

Understood. But we don't need to check it.

For key_UP, it can do like:
If ( --p_shell->selected_index < 0 )
p_shell->selected_index += p_shell->total_buffered_cmds;

[Cao, Minggui] there is a rule for history cmds: it is not a loop,
if press up or down key always, it will go up/down to the first or
last command as the end one, so the user will know the history is to end.
1. if the ring buffer not full, up key will go until 0 index, then
it will not change any more.
2. if the ring buffer is full, up key will also go to an end index
too and will not change any more then.

Why we need this kind of difference? I am not convinced/

[Cao, Minggui] OK, let me explain in detailed, Let me give two examples:

1. if ring buffer full is 8, now buffered index: 0, 1, 2, 3, total_buffered_cmds is 4;
if up key on and on, it will go as:
3-->2-->1-->0, and then if up key again, it keeps as 0, and not change
again.
2. if ring buffer is full, and buffered index order is: 4, 5, 6, 7, 0, 1, 2 ,3, and start
from 3, for up keys, it will go as:
3->2->1->0->7->6->5->4, and then keeps as 4, not change again for
more up keys.

When I check bash, found it is not a loop, I think the reason is user-friendly:
user can know when it is the end of the history buffered cmds.

As the two cases, I have not found how to unify the code; if adopt the above
code, it is a loop, it is fine, but not user-friendly, the user could search more
time.

I updated this patch to V3, and sent it out, combine "input_line"
and "buffered_cmds" as "buffered_line", and simplify some logic as
your

comments.

Could you have a review?