sys/shell: correctly detect and handle long lines #13195

HendrikVE · 2020-01-24T13:10:53Z

Contribution description

With this PR I simply copied PR #10635 authored by @jcarrano. Since he left the team, I hereby offer to take care of rebasing and implement required changes.

The numeric value for EOF is (-1). This caused the shell to return the same code when EOF was encountered and when the line length was exceeded. Additionally, if the line length is exceeded, the correct behaviour is to consume the remaining characters until the end of the line, to prevent the following line from containing (potentially dangerous) garbage.

Testing procedure

This PR adds a test to tests/shell. It adds a command to retrieve buffer size and then forces an extremely long line. This is failing right now because of a bug in the shell, which is fixed with this PR.

In case of problems with testing on a real board, please refer to #10639 first.

Issues/PRs references

Copy of #10635

Dependency for #13196 and #13197

tests/shell/tests/01-run.py

…RIOT-OS#13195) The numeric value for EOF is -1. This caused the shell to return the same code when EOF was encountered and when the line lenght was exceeded. Additionally, if the line length is exceeded, the correct behaviour is to consume the remaining characters until the end of the line, to prevent the following line from containing (potentially dangerous) garbage. Co-authored-by: Hendrik van Essen <[email protected]> sys/shell: rephrase/reformat some comments tests/shell: add test case for shell's buffer size Co-authored-by: Juan Carrano <[email protected]> tests/shell: avoid sending an extra empty line on native Co-authored-by: Juan Carrano <[email protected]> tests/shell: check for startup message Co-authored-by: Juan Carrano <[email protected]> tests/shell: check for shell prompt Co-authored-by: Juan Carrano <[email protected]> tests/shell: fix test case for line cancelling The test for the line cancellation (ctrl-c) functionality was unable to detect error because of the way pexpect matches output. While working on the long line shell bug, a regression was about to be introduced because of this. This commit fixes the test by directly reading from the child process and expects an exact response. Co-authored-by: Juan Carrano <[email protected]> tests/shell: add test case for exceeding lines Co-authored-by: Juan Carrano <[email protected]> tests/shell: remove redundant parentheses

RIOT-OS#13195) The numeric value for EOF is -1. This caused the shell to return the same code when EOF was encountered and when the line lenght was exceeded. Additionally, if the line length is exceeded, the correct behaviour is to consume the remaining characters until the end of the line, to prevent the following line from containing (potentially dangerous) garbage. Co-authored-by: Hendrik van Essen <[email protected]> sys/shell: rephrase/reformat some comments tests/shell: add test case for shell's buffer size Co-authored-by: Juan Carrano <[email protected]> tests/shell: avoid sending an extra empty line on native Co-authored-by: Juan Carrano <[email protected]> tests/shell: check for startup message Co-authored-by: Juan Carrano <[email protected]> tests/shell: check for shell prompt Co-authored-by: Juan Carrano <[email protected]> tests/shell: fix test case for line cancelling The test for the line cancellation (ctrl-c) functionality was unable to detect error because of the way pexpect matches output. While working on the long line shell bug, a regression was about to be introduced because of this. This commit fixes the test by directly reading from the child process and expects an exact response. Co-authored-by: Juan Carrano <[email protected]> tests/shell: add test case for exceeding lines Co-authored-by: Juan Carrano <[email protected]> tests/shell: remove redundant parentheses

HendrikVE · 2020-02-24T14:37:56Z

@smlng @kaspar030 Since you guys already had a look at #10635, you may also have (instead) a look at this successor PR please? :)

fjmolinas

Thanks for taking over @HendrikVE! Some styling comments, otherwise looks good. Will test asap.

fjmolinas · 2020-03-02T08:22:11Z

sys/shell/shell.c

 #ifndef SHELL_NO_ECHO
            _putchar('\r');
            _putchar('\n');
 #endif

-            /* return 1 if line is empty, 0 otherwise */
-            return c == ETX || line_buf_ptr == buf;
+            return (length_exceeded)? ERROR_READLINE_EXCEEDED : curr_pos;


Suggested change

return (length_exceeded)? ERROR_READLINE_EXCEEDED : curr_pos;

return (length_exceeded) ? ERROR_READLINE_EXCEEDED : curr_pos;

Gonna add the space :)

sys/shell/shell.c

fjmolinas · 2020-03-02T08:59:41Z

sys/shell/shell.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009, Freie Universitaet Berlin (FUB).
+ * Copyright (C) 2020 Freie Universität Berlin


I have no idea here, but can copyright be updated like this? Should both years?

sys/shell/shell.c

fjmolinas · 2020-03-02T10:11:16Z

Since shell is used in a lot of tests I will trigger running all tests to be sure.

fjmolinas

I have some comments on the test script and on the new prints.

fjmolinas · 2020-03-03T13:36:08Z

tests/shell/tests/01-run.py

+        # looks like the nrf52dk runs in to undefined behaviour when sending more
+        # than 64 bytes over UART


Lets still skip the test but improve the message, feel free to improve, but something like:

Suggested change

# looks like the nrf52dk runs in to undefined behaviour when sending more

# than 64 bytes over UART

# There is an issue with nrf52dk when the Virtual COM port is connected and sending more than 64 bytes over

# UART. If no terminal is connected to the Virtual COM and interfacing directly to the nrf52832 UART pins the

# issue is not present.

fjmolinas · 2020-03-03T13:41:13Z

tests/shell/tests/01-run.py

    child.sendline(cmd)
    for line in expected:
        child.expect_exact(line)


+def check_startup(child):
+    child.expect('test_shell.\r\n')


This will make it work for boards even if there is no reset called before starting the test (some boards can't be reset with make reset)

Suggested change

child.expect('test_shell.\r\n')

child.sendline(CONTROL_C)

child.expect_exact(PROMPT)

Cool, thank you. I totally ignored the possibility ^^

What do you think about using test_utils_interactive_sync() as introduced in 1cb8bd4 ?

fjmolinas · 2020-03-03T14:41:49Z

sys/shell/shell.c

+                break;
+
+            case 0:
+                puts("shell: line is empty");


I think this should be removed, a terminal should not print a message on an empty line, it wasn't the case before.

Suggested change

puts("shell: line is empty");

sys/shell/shell.c

fjmolinas · 2020-03-28T13:08:23Z

Ok, so the CI has some failed tests, but all of them are timeouts for nrf52dk.

I think they are unrelated to your changes. But we did change the terminal to use socat instead of picocom, and I wonder if that change caused instability for nrf52dk? I would say your changes are indeed unrelated to those failures though. The failed tests have not been the same in the different runs either. And I've seen that on other PR's as well.

I will need to investigate that. Because IMO it might be related to the change to socat. But this might as well just be exposing a bug in nrf52dk. If for some reason we are forced to change back to picocom we can remove theassert on the garbage test, until we figure things out. but lets not delay this any further.

The numeric value for EOF is -1. This caused the shell to return the same code when EOF was encountered and when the line lenght was exceeded. Additionally, if the line length is exceeded, the correct behaviour is to consume the remaining characters until the end of the line, to prevent the following line from containing (potentially dangerous) garbage. Co-authored-by: Hendrik van Essen <[email protected]>

Co-authored-by: Juan Carrano <[email protected]>

The test for the line cancellation (ctrl-c) functionality was unable to detect error because of the way pexpect matches output. While working on the long line shell bug, a regression was about to be introduced because of this. This commit fixes the test by directly reading from the child process and expects an exact response. Co-authored-by: Juan Carrano <[email protected]>

Co-authored-by: Juan Carrano <[email protected]>

fjmolinas · 2020-03-30T10:29:07Z

@HendrikVE I rebased and fixed Makefile.ci, I pushed directly to your branch. I hope that is OK.

fixed shell is too big for atmega32u4 based boards

fjmolinas

ACK, fixes the issue, we ran all tests on murdock and although there where failures they are unrelated.

fjmolinas · 2020-03-30T11:37:13Z

GO!, Thanks for the good work and the patience @HendrikVE! as well as taking over for @jcarrano

HendrikVE · 2020-03-30T12:04:49Z

Thank you very much for your review @fjmolinas, I really appreciate that! Also thanks @jcarrano for your initial work ;)

HendrikVE mentioned this pull request Jan 24, 2020

sys/shell: correctly detect and handle long lines #10635

Closed

HendrikVE force-pushed the shell-fix-longcmd branch from 12af92c to ce3ebb4 Compare February 7, 2020 12:26

This was referenced Feb 7, 2020

sys/shell: refactor readline function #13196

Merged

sys/shell: refactor tokenizer code #13197

Merged

HendrikVE commented Feb 7, 2020

View reviewed changes

tests/shell/tests/01-run.py Show resolved Hide resolved

HendrikVE mentioned this pull request Feb 7, 2020

sys/stdio_uart: dropped data when received at once #10639

Closed

HendrikVE force-pushed the shell-fix-longcmd branch from ce3ebb4 to f6d093f Compare February 9, 2020 23:07

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 10, 2020

HendrikVE force-pushed the shell-fix-longcmd branch from f6d093f to 4944089 Compare February 10, 2020 12:26

HendrikVE added Area: tests Area: tests and testing framework Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Feb 10, 2020

fjmolinas reviewed Mar 2, 2020

View reviewed changes

fjmolinas requested a review from kaspar030 March 2, 2020 09:13

fjmolinas added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 2, 2020

fjmolinas requested changes Mar 3, 2020

View reviewed changes

HendrikVE requested a review from aabadie as a code owner March 4, 2020 12:00

jcarrano and others added 9 commits March 30, 2020 12:26

sys/shell: rephrase/reformat some comments

3860355

tests/shell: add test case for shell's buffer size

c8daf59

Co-authored-by: Juan Carrano <[email protected]>

tests/shell: avoid sending an extra empty line on native

393eadc

Co-authored-by: Juan Carrano <[email protected]>

tests/shell: check for startup message

32d7c9b

Co-authored-by: Juan Carrano <[email protected]>

tests/shell: check for shell prompt

c1e3e61

Co-authored-by: Juan Carrano <[email protected]>

tests/shell: add test case for exceeding lines

68a35f5

Co-authored-by: Juan Carrano <[email protected]>

tests/shell: remove redundant parentheses

a9b21ed

fjmolinas force-pushed the shell-fix-longcmd branch from 8f1777c to 574c173 Compare March 30, 2020 10:28

tests/driver_pca9685: add atmega32u4 boards to Makefile.ci

e38e3c5

fixed shell is too big for atmega32u4 based boards

fjmolinas force-pushed the shell-fix-longcmd branch from 574c173 to e38e3c5 Compare March 30, 2020 10:35

fjmolinas approved these changes Mar 30, 2020

View reviewed changes

fjmolinas merged commit e738508 into RIOT-OS:master Mar 30, 2020

leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020

HendrikVE deleted the shell-fix-longcmd branch June 26, 2020 09:35

fjmolinas mentioned this pull request Jul 2, 2020

OSX + SAMR21-xpro: shell cannot handle command inputs larger than 64 chars #5863

Closed

aabadie mentioned this pull request May 20, 2021

[TRACKING] The RIOT shell as a testing interface #10994

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/shell: correctly detect and handle long lines #13195

sys/shell: correctly detect and handle long lines #13195

HendrikVE commented Jan 24, 2020 •

edited

Loading

HendrikVE commented Feb 24, 2020

fjmolinas left a comment

fjmolinas Mar 2, 2020

HendrikVE Mar 2, 2020

fjmolinas Mar 2, 2020

HendrikVE Mar 2, 2020

fjmolinas Mar 2, 2020

fjmolinas commented Mar 2, 2020

fjmolinas left a comment

fjmolinas Mar 3, 2020

fjmolinas Mar 3, 2020

HendrikVE Mar 4, 2020

HendrikVE Mar 4, 2020

fjmolinas Mar 3, 2020

fjmolinas commented Mar 28, 2020

fjmolinas commented Mar 30, 2020

fjmolinas left a comment

fjmolinas commented Mar 30, 2020

HendrikVE commented Mar 30, 2020

	return (length_exceeded)? ERROR_READLINE_EXCEEDED : curr_pos;
	return (length_exceeded) ? ERROR_READLINE_EXCEEDED : curr_pos;

		# looks like the nrf52dk runs in to undefined behaviour when sending more
		# than 64 bytes over UART

-        # looks like the nrf52dk runs in to undefined behaviour when sending more
-        # than 64 bytes over UART
+        # There is an issue with nrf52dk when the Virtual COM port is connected and sending more than 64 bytes over
+        # UART. If no terminal is connected to the Virtual COM and interfacing directly to the nrf52832 UART pins the
+        # issue is not present.

	child.expect('test_shell.\r\n')
	child.sendline(CONTROL_C)
	child.expect_exact(PROMPT)

sys/shell: correctly detect and handle long lines #13195

sys/shell: correctly detect and handle long lines #13195

Conversation

HendrikVE commented Jan 24, 2020 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

HendrikVE commented Feb 24, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Mar 2, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Mar 28, 2020

fjmolinas commented Mar 30, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas commented Mar 30, 2020

HendrikVE commented Mar 30, 2020

HendrikVE commented Jan 24, 2020 •

edited

Loading