sys/shell: Exit the shell on ctrl-D #10788

jcarrano · 2019-01-16T16:51:33Z

Contribution description

Note the control-C part of this PR was split to #11004 .

Right now the only way to exit the shell is if stdin is closed. This works on native, but on an embedded platform stdin is the uart and thus is never closed.

This patch causes the shell loop to exit on EOT (ASCII 0x04 / ctrl-D a.k.a. "End-of-Transmission".)

Testing procedure

Run the test in tests/shell with native and with a board.

$ cd tests/shell
$ BOARD=samr21-xpro make all
$ BOARD=samr21-xpro make flash
$ BOARD=samr21-xpro make test

To test it by hand with a board, use a terminal program that passes ctrl-D to the tty. In native, use Ctrl-V ctrl-D to insert a literal EOT.

Related PRs

Depends on #11003 , #11004.

kaspar030 · 2019-01-16T16:58:05Z

on an embedded platform

use case?

jcarrano · 2019-01-16T17:02:17Z

use case?

Examples, test scripts. In my case I was trying to integrate more tests in to tests/shell and needed to exit the shell.

Right now the shell is an infinite loop (except in native). I believe there should be a way to get out.

kaspar030 · 2019-01-16T17:05:49Z

needed to exit the shell.

Why?

miri64 · 2019-01-17T10:40:24Z

I don't know about this. I sometimes use the fact in my testing, that the node is still accessible with make term after I Strg+C out of a previous make term.

Fix the use of python dictionaries: Python dictionaries are not guaranteed to be ordered until version
3.7. In 3.6 they are ordered too, but that is an implementation detail. riotdocker seems to be using 3.5. The commands in this test were stored in a dict.

There are also OrderedDict.

jcarrano · 2019-01-17T10:47:16Z

I don't know about this. I sometimes use the fact in my testing, that the node is still accessible with make term after I Strg+C out of a previous make term.

If you have a recent version of python everything should be OK. In 3.7 it is even documented that they preserver insertion order.

There are also OrderedDict.

Yes, but the test here was only using a dict to store pairs and not doing indexing of any kind. In that case a list of 2-tuples is the best choice.

miri64 · 2019-01-17T10:50:19Z

I don't know about this. I sometimes use the fact in my testing, that the node is still accessible with make term after I Strg+C out of a previous make term.

If you have a recent version of python everything should be OK. In 3.7 it is even documented that they preserver insertion order.

I don't see how this helps me with the shell being closed and me not being able to access the node anymore...

miri64 · 2019-01-17T10:50:55Z

(unless I reset)

jcarrano · 2019-01-17T10:59:48Z

I don't see how this helps me with the shell being closed and me not being able to access the node anymore...

Sorry, I messed up the comments.
This should not break your test. If you are using pyterm, Ctrl-C gets caught by pyterm, and causes pyterm to exit. In fact, it seems you cannot send through the Ctrl-C/D using pyterm.

miri64 · 2019-01-17T11:03:05Z

This should not break your test. If you are using pyterm, Ctrl-C gets caught by pyterm, and causes pyterm to exit. In fact, it seems you cannot send through the Ctrl-C/D using pyterm.

This still leaves the node in an "unexpected" state. If I reconnect with a terminal I expect the node still to do whatever it was doing before (unless I toggle the reset pin), not being able to interact with it anymore, because the shell isn't there anymore.

You still don't answered the question why the shell needs to be exited.

jcarrano · 2019-01-17T11:26:05Z

This still leaves the node in an "unexpected" state. If I reconnect with a terminal I expect the node still to do whatever it was doing before

It will. You can try it out. If CTRL-C exits the TERMPROG, it means that TERMPROG caught it and it was not sent to the node, and the shell never exited.

Exiting the shell allows one to write a test like the one in #9733 where the first part is interactive, then the shell exits and a non interactive part is run. Of course, the non interactive part can be first, but then the test starts immediately after the node boots, and we know this brings synchronization issues.

Also, if the shell can exit, but one does not want it to, it is easy to fix (wrap it in a while loop). The other case (it does not exit, but one wants it to) is not.

Observe that the shell can be exited now, so this is not adding the possibility of exiting, just another way to do it:

RIOT/sys/shell/shell.c

Lines 229 to 232 in 6ed11de

    
           int c = getchar(); 
        
           if (c < 0) { 
        
               return EOF; 
        
           }

RIOT/sys/shell/shell.c

Lines 290 to 292 in 6ed11de

    
           if (res == EOF) { 
        
               break; 
        
           }

Lastly: why the shell needs not to exit? I honestly don't understand why we are bikeshedding a <1 line change. Look at the asymmetry between the reliance we have on the shell (especially for tests) and the quality of the module and associated tools (like pyterm.) I'm doing a series of PRs trying to improve the situation (#10630, #10635, #9733) and I have other PRs on the works and I keep running into the same endless bikeshedding. Look at this change: it add 74 lines of code: one is the change itself, the remaining 73 are tests: tests that test all, and that run without interaction. I cannot help but wonder why the long discussion, when stuff with much less testing and more poorly described gets in way more easily.

kaspar030 · 2019-01-17T11:40:35Z

Lastly: why the shell needs not to exit? I honestly don't understand why we are bikeshedding a <1 line change.

This is not bikeshedding. You stated "In my case I was trying to integrate more tests in to tests/shell and needed to exit the shell." and I asked to clarify why you need to exit the shell.

Even a one line change needs to be justified, especially in something that was in the same shape for almost a decade.

I'm not opposed to the change, just wondering why on earth you need to exit the shell in order to add more tests, and if there's not a simpler solution.

jnohlgard · 2019-01-18T22:27:56Z

I am against this change, there are sometimes a lot of noise on the UART when reseting, connecting the wires etc. Normally there is little risk of the shell interpreting the noise as a real command, but when the shell closes after a single (or two separate) specific byte it gets problematic.

benemorius · 2019-01-19T01:51:13Z

I use an 'exit' command to close the shell. ^D is nice but I typed it too often without thinking and then my shell was gone. But it's true that wrapping it in a while loop solves this for applications where you don't want the shell to ever go away (for instance I think everything currently in examples would qualify). This would mimic how getty works on linux. You can always exit the shell, but by default it is configured to respawn immediately.

I've needed to close the shell too. You have to if you want to use the uart for something else. I telnet in over the wireless interface and get a shell there, then run a command to kill the other shell running on the uart and redirect the uart through the telnet terminal. In this way I use a riot board as a wireless ftdi cable. Afterwards I respawn the shell on the uart.

This is one use case for closing the shell but certainly there are others.

benemorius · 2019-01-23T18:23:22Z

sys/shell/shell.c

@@ -227,7 +227,7 @@ static int readline(char *buf, size_t size)
        }

        int c = getchar();
-        if (c < 0) {
+        if (c < 0 || c == '\x03' || c == '\x04') {


I don't think it's right to exit on ^C. We should cancel the current line and print a new prompt. This is consistent with how bash and friends behave. Like this: benemorius/RIOT@a946c44

Thanks for the suggestion. I have implemented it, I think it is more useful.

jcarrano · 2019-02-12T12:47:05Z

My interpretation of the comments is that what's contested here is exiting with control-D. Cancelling with control-C should not be as controversial, right?

I will split this PR.

stale · 2019-08-16T15:54:50Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

Right now the only way to exit the shell is if stdin is closed. This works on native, but on an embedded platform stdin is the uart and thus is never closed. This patch causes the shell loop to exit on EOT (ASCII 0x04 / ctrl-D), also called "End-of-Transmission".

Test that the shell exits on ctrl-D and that it exits only once.

jcarrano · 2019-09-10T12:59:02Z

There's a couple of uses for this functionality which I had not thought of before:

Simple user/pass login: One could put the shell in a while(1) loop, preceded by a login prompt function. This would only drop you to the shell if you had the correct user/password. After using the shell you go back to the login prompt by typing ctrl-d.
What happens if the file object used by stdin is closed while there is a thread waiting on getchar? The shell gets an EOF and exits. This can happen if stdio is implemented in some way other than the UART (e.g. bluetooth). There we can see that the shell exiting is not such an exceptional circumstance.

benemorius · 2019-09-19T06:41:54Z

After re-reading my own comments I should clarify that I'm strongly in favor of exiting on ^D. We just need to wrap existing consumers of the shell with a while loop to avoid functional changes in existing code which doesn't wish to have a shell capable of exiting.

benemorius · 2019-09-19T06:45:13Z

Does wrapping the shell in a while loop solve everyone's concerns with exiting on ^D?

miri64 · 2019-09-19T06:58:41Z

Does wrapping the shell in a while loop solve everyone's concerns with exiting on ^D?

So this is how shell_run() currently looks like:

RIOT/sys/shell/shell.c

Lines 293 to 310 in 7118545

    
           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); 
        
                   } 
        
                   print_prompt(); 
        
               } 
        
           }

Currenlty, ^D is just ignored (handle_input_line() sees *pos <= ' ' so nothing happens). The functionality introduced here causes the function to go into the if condition and break that loop. Adding another while loop around the existing while loop however would have the same behavior, just with another while loop around: Hitting ^D would finish the inner while loop, the outer while loop would then cause another iteration of the inner while loop. So I don't really get the point of having this then except for the handle_input_line() not being entered.

benemorius · 2019-09-19T07:16:45Z

Ok so when I say to wrap existing consumers of the shell with a while loop I don't mean adding a loop to shell.c::shell_run() but rather to examples/default/main.c::main() for example.

The idea is that we can make the shell optionally exitable but then modify all the existing callers of shell_run() to restore the current-master non-exitable behavior for whichever cases we don't actually want the shell to ever exit.

It's equivalent to the way that in linux getty calls bash, and whenever bash exits, getty just calls bash again, forever. bash doesn't re-call itself. It's getty that does that.

miri64 · 2019-09-19T07:25:19Z

Ok, got it, but to quote yourself:

We just need to wrap existing consumers of the shell with a while loop to avoid functional changes in existing code which doesn't wish to have a shell capable of exiting.

would mean to avoid functional changes we need to touch every call of shell_run then... Given that we use that a lot, this seems somewhat of a big effort to me. As a compromise: how about introducing shell_run_once() to the shell's with the current functionality of shell_run() (+ the one introduced in this PR) and moving shell_run() to a function that calls shell_run_once() in an infinite loop?

benemorius · 2019-09-19T07:42:36Z

Ok, got it, but to quote yourself:

We just need to wrap existing consumers of the shell with a while loop to avoid functional changes in existing code which doesn't wish to have a shell capable of exiting.

would mean to avoid functional changes we need to touch every call of shell_run

Yes, correct.

I mean grep says it's not that many changes and all of them are simply adding a while(1) loop which should be trivial to review. We do need an exitable shell. End users can make an exitable shell non-exitable but they cannot make a non-exitable shell exitable.

As a compromise: how about introducing shell_run_once() to the shell's with the current functionality of shell_run() (+ the one introduced in this PR) and moving shell_run() to a function that calls shell_run_once() in an infinite loop?

At a glance, I have no objection to this.

miri64 · 2019-09-19T07:50:23Z

I mean grep says it's not that many changes and all of them are simply adding a while(1) loop which should be trivial to review.

Nevertheless, it's a moving target, as new tests or examples might introduce additional calls. In my experience those are the real trouble, as behavioral changes like this might take a while to settle for devs used to the current behavior.

This change is in preparation to [PR 10788]. PR 10788 will make the shell exitable which may lead to unexpected behavior in comparison to previous usage of the shell. To prevent this, this PR introduces two "new" functions to the shell's API: `shell_run_once()` and `shell_run_forever()`. `shell_run_once()` basically has the same behavior as `shell_run()` in current master: Start a shell and continue reading lines until EOF is reached. `shell_run_forever()` wraps around `shell_run_once()` and restarts the shell if it exits. `shell_run()` is re-introduced as a back-porting alias for `shell_run_forever()`. As a consequence all current calls to `shell_run()` won't exit even with [PR 10788] merged (which would add EOT as additional exit condition for `shell_run_once()`). [PR 10788]: RIOT-OS#10788

miri64 · 2019-09-19T08:36:29Z

As a compromise: how about introducing shell_run_once() to the shell's with the current functionality of shell_run() (+ the one introduced in this PR) and moving shell_run() to a function that calls shell_run_once() in an infinite loop?

At a glance, I have no objection to this.

See #12272 for a demonstrator of what I mean.

jcarrano · 2019-09-20T15:55:13Z

One thing I must point out is that ctrl-D handling in native and on serial based targets is different (and that should be fixed some day).

Native uses the terminal in cooked mode, so it will never see the ctrl-d, instead it sees an EOF.

Embedded targets use what would be the equivalent of raw mode, thus seen ctrl-d, ctrl-c, etc as what they are (though depending on the terminal program you are using you may have to use Ctrl-V to prevent it from being interpreted by your local machine).

IMO this discrepancy should be fixed, and native should be setting up the terminal to behave as close to the UART as possible (raw mode, no buffering, no echo) using tcsetattr and friends.

All this being said, it may make sense to differentiate between the stream being closed and a ctrl-d (maybe do a feof() before returning from shell_run? or add an additional code to handle_input_line?) and change the return type of shell run to allow it to report this.

jcarrano · 2019-09-20T15:57:54Z

... shell_run_once() ...
.. moving shell_run() to a function that calls shell_run_once() ...

you can do this, but why invent new "sentences" when you can express that same logic using the vocabulary already present in C (while (stream not closed) { ... shell_run(...) .. })

This change is in preparation to [PR 10788]. PR 10788 will make the shell exitable which may lead to unexpected behavior in comparison to previous usage of the shell. To prevent this, this PR introduces two "new" functions to the shell's API: `shell_run_once()` and `shell_run_forever()`. `shell_run_once()` basically has the same behavior as `shell_run()` in current master: Start a shell and continue reading lines until EOF is reached. `shell_run_forever()` wraps around `shell_run_once()` and restarts the shell if it exits. `shell_run()` is re-introduced as a back-porting alias for `shell_run_forever()`. As a consequence all current calls to `shell_run()` won't exit even with [PR 10788] merged (which would add EOT as additional exit condition for `shell_run_once()`). [PR 10788]: RIOT-OS#10788

stale · 2020-03-23T16:02:59Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 · 2020-03-23T20:52:45Z

Alternative approach provided in #13675.

benpicco · 2020-03-25T10:48:47Z

@miri64 this is unrelated.
On native the shell never receives the raw EOT. The signal is caught by the operating system and results in closing stdin, so the shell will exit.
This PR is about replicating this behavior on the target.

miri64 · 2020-03-26T22:10:43Z

Ok

fjmolinas · 2020-06-23T10:53:31Z

Now that the examples uses shell_run() can we move forward with this one? @benemorius are you still interested?, @HendrikVE you have been modifying the shell a lot lately?

HendrikVE · 2020-06-24T13:19:32Z

@fjmolinas Yes, right now ctrl-D is not caught at all, leading to consuming it as a normal character. This is not good, it should at least be caught and ignored. But now, as we have the infinite loop wrapper around the shell it might be less controversal to exit the shell with ctrl-D?

HendrikVE · 2020-06-24T13:40:57Z

With the new shell it would look like #14345

miri64 · 2020-06-24T15:52:41Z

Then let's close this?

jcarrano added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 16, 2019

jcarrano mentioned this pull request Jan 16, 2019

sys/shell: Add function to call the shell with a list of strings. #9733

Closed

benemorius reviewed Jan 23, 2019

View reviewed changes

jcarrano changed the title ~~sys/shell: Exit the shell on ctrl-D or ctrl-C.~~ sys/shell: Exit the shell on ctrl-D and cancel line on ctrl-C. Feb 8, 2019

This was referenced Feb 12, 2019

tools/serial.inc.mk: Support miniterm.py. #11003

Closed

sys/shell: cancel current line on CTRL-C. #11004

Merged

jcarrano force-pushed the shell_exit_ctrl_d branch from c434864 to 23eb783 Compare February 12, 2019 15:00

jcarrano changed the title ~~sys/shell: Exit the shell on ctrl-D and cancel line on ctrl-C.~~ sys/shell: Exit the shell on ctrl-D Feb 12, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 16, 2019

jcarrano mentioned this pull request Aug 28, 2019

[TRACKING] sys/shell refactoring. #12105

Open

8 tasks

jcarrano force-pushed the shell_exit_ctrl_d branch from 23eb783 to f56eb5b Compare September 10, 2019 11:56

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 10, 2019

jcarrano added 2 commits September 10, 2019 13:59

tests/shell: Test exit with control-D.

f56eb5b

Test that the shell exits on ctrl-D and that it exits only once.

jcarrano mentioned this pull request Sep 10, 2019

examples/default: PoC user/password login for the terminal #12191

Closed

miri64 mentioned this pull request Sep 19, 2019

shell: make shell_run run shell forever #12272

Merged

aabadie mentioned this pull request Mar 22, 2020

sys/shell: native: stop RIOT when the shell reads EOF #13675

Merged

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 23, 2020

miri64 closed this Mar 23, 2020

miri64 reopened this Mar 26, 2020

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 26, 2020

HendrikVE mentioned this pull request Jun 24, 2020

sys/shell: terminate shell on Ctrl-D #14345

Merged

miri64 closed this Jun 24, 2020

sys/shell: Exit the shell on ctrl-D #10788

sys/shell: Exit the shell on ctrl-D #10788

Conversation

jcarrano commented Jan 16, 2019 • edited Loading

Contribution description

Testing procedure

Related PRs

kaspar030 commented Jan 16, 2019

jcarrano commented Jan 16, 2019

kaspar030 commented Jan 16, 2019

miri64 commented Jan 17, 2019

jcarrano commented Jan 17, 2019

miri64 commented Jan 17, 2019

miri64 commented Jan 17, 2019

jcarrano commented Jan 17, 2019

miri64 commented Jan 17, 2019

jcarrano commented Jan 17, 2019

kaspar030 commented Jan 17, 2019

jnohlgard commented Jan 18, 2019

benemorius commented Jan 19, 2019

benemorius Jan 23, 2019

Choose a reason for hiding this comment

jcarrano Feb 7, 2019

Choose a reason for hiding this comment

jcarrano commented Feb 12, 2019

stale bot commented Aug 16, 2019

jcarrano commented Sep 10, 2019

benemorius commented Sep 19, 2019 • edited Loading

benemorius commented Sep 19, 2019

miri64 commented Sep 19, 2019

benemorius commented Sep 19, 2019 • edited Loading

miri64 commented Sep 19, 2019

benemorius commented Sep 19, 2019

miri64 commented Sep 19, 2019

miri64 commented Sep 19, 2019

jcarrano commented Sep 20, 2019

jcarrano commented Sep 20, 2019

stale bot commented Mar 23, 2020

miri64 commented Mar 23, 2020

benpicco commented Mar 25, 2020

miri64 commented Mar 26, 2020

fjmolinas commented Jun 23, 2020

HendrikVE commented Jun 24, 2020 • edited Loading

HendrikVE commented Jun 24, 2020

miri64 commented Jun 24, 2020

jcarrano commented Jan 16, 2019 •

edited

Loading

benemorius commented Sep 19, 2019 •

edited

Loading

benemorius commented Sep 19, 2019 •

edited

Loading

HendrikVE commented Jun 24, 2020 •

edited

Loading