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


Minggui Cao
 

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

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?




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

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