sys/shell: terminate shell on Ctrl-D#14345
Conversation
HendrikVE
requested review from
MichelRottleuthner,
aabadie,
fjmolinas,
leandrolanzieri,
miri64 and
smlng
as code owners
HendrikVE
added
Area: sys
| shell_run_once(shell_commands, line_buf, SHELL_DEFAULT_BUFSIZE); | ||
|
|
||
| puts("shell exited"); | ||
|
|
||
| /* Restart the shell after the previous one exits, so that we can test | ||
| * Ctrl-D exit */ | ||
| shell_run(shell_commands, line_buf, SHELL_DEFAULT_BUFSIZE); |
There was a problem hiding this comment.
I would run this in a loop instead.
There was a problem hiding this comment.
Also in native I get an infinite loop, can we do something simililar as for #13675?
There was a problem hiding this comment.
Two different behaviours are tested here. When we do ctrl-d in shell_run_once the execution returns to main of the test program. If we do ctrl-d in shell_run the shell will be respawn. I added a second test case for ctrl-d and comments to emphasize this behaviour. Do you still get an infinite loop?
|
Sorry @HendrikVE I completely lost track of this one, can you rebase? |
HendrikVE
force-pushed
the
pr/shell-exit-on-ctrl-d
branch
from
b0d99f8 to
47efe80
Compare
|
I rebased this PR :) |
|
Solves the issue, please squash @HendrikVE! |
fjmolinas
added
the
CI: run tests
HendrikVE
force-pushed
the
pr/shell-exit-on-ctrl-d
branch
from
47efe80 to
8b908e2
Compare
| ('echo \t tabs\t\t processed \t\tlike\t \t\tspaces', '"echo""tabs""processed""like""spaces"'), | ||
|
|
||
| # test long line | ||
| # test unknown commands |
There was a problem hiding this comment.
This change is unrelated and the initial comment was correct (the purpose of the test line 58 is to test long lines). But the new comment could be kept and moved before line 62 (since it makes sense for this one).
There was a problem hiding this comment.
It's not really testing long lines here, because these test strings have a fixed length. The test done by calling check_line_exceeded is better suited for this purpose, because it makes use of the shell's buffer size automatically and writes characters beyond the buffer size. Maybe we should replace the long sequence of numbers by a shorter one or by something completely different instead?
There was a problem hiding this comment.
The test done by calling check_line_exceeded is better suited for this purpose, because it makes use of the shell's buffer size automatically and writes characters beyond the buffer size.
If this case is already covered by another test then I would suggest to remove this case and only keep 'unknown_command' one. @fjmolinas what do you think ?
There was a problem hiding this comment.
Well its still an unrelated change, even if it makes sense, maybe you could split those into a separate commit @HendrikVE ?
There was a problem hiding this comment.
It's in a separate commit now
benpicco
removed
the
CI: run tests
|
Only unrelated failures:
|
Ctrl-D was not caught in a special case so it was interpreted as a standard character. Handle it now the same way like EOF and terminate the shell instance.
One test was removed, because the intended test case is already covered by check_line_exceeded
HendrikVE
force-pushed
the
pr/shell-exit-on-ctrl-d
branch
from
8b908e2 to
7a775ec
Compare
|
Looks good, I'll do a small test run and ACK. |
|
I ran the test on a collection of boards, only failure on |
4 participants
Ctrl-D was not caught in a special case so it was interpreted as a standard character. Handle it now the same way like EOF and terminate the shell instance.
Related to #10788