[riot-notifications] [RIOT-OS/RIOT] sys/shell: correctly detect and handle long lines (#10635)

Sebastian Meiling notifications at github.com
Fri Jun 28 22:50:46 CEST 2019


smlng requested changes on this pull request.



> @@ -20,4 +20,7 @@ TEST_ON_CI_WHITELIST += all
 
 APP_SHELL_FMT ?= NONE
 
+# needed to correctly test ling lines

/ling/long/ ?

>  static const shell_command_t shell_commands[] = {
+    { "bufsize", "Get the shell's buffer size", print_shell_bufsize },

maybe plain and simple: "Get shell buffer size"?

>  void shell_run(const shell_command_t *shell_commands, char *line_buf, int len)
 {
     print_prompt();
 
     while (1) {
         int res = readline(line_buf, len);
 
-        if (res == EOF) {
-            break;
-        }
-
-        if (!res) {
-            handle_input_line(shell_commands, line_buf);
+        switch (res) {
+            case EOF:
+                goto shell_run_exit;

there are (better) places where `goto` has its (controversial) benefits - here it looks rather cosmetic (to me) and I would go for a simple `return;` as @kaspar030 suggested. 


>  void shell_run(const shell_command_t *shell_commands, char *line_buf, int len)
 {
     print_prompt();
 
     while (1) {
         int res = readline(line_buf, len);
 
-        if (res == EOF) {
-            break;
-        }
-
-        if (!res) {
-            handle_input_line(shell_commands, line_buf);
+        switch (res) {

as there is no other use of `res` could be replace with `switch (readline(line_buf, len)) {`

>          }
 
         print_prompt();
     }
+
+shell_run_exit:

removing `goto` make these 2 lines obsolete

> @@ -50,7 +50,18 @@ def check_cmd(child, cmd, expected):
 
 def testfunc(child):
     # check startup message
-    child.expect('test_shell.')
+    child.expect('test_shell.\r')
+
+    child.sendline('bufsize')
+    child.expect('([0-9]+)')
+
+    bufsize = int(child.match[1])
+
+    # check a long line
+    child.sendline("_"*bufsize + "whatever")

maybe use `foobar` or `42` instead 🤓   

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10635#pullrequestreview-255956915
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190628/932afe90/attachment.html>


More information about the notifications mailing list