Do not try to continue reading shell commands if input source is closed by x3ro · Pull Request #10105 · RIOT-OS/RIOT

x3ro · 2018-10-02T16:18:39Z

Contribution description

In RIOT native, sending CTRL+D to a shell started using shell_run would resulted in and
endless prompt loop. I've been unable to trigger such a behaviour
on actual hardware using a UART connection, but calling pm_off seemed
like a better alternative than having an #ifdef BOARD_NATIVE.

Testing procedure

From the Tutorials repo, compile and run task01 for native.
Press CTRL-D in your terminal (this sends EOF)

Expected:

The shell exits cleanly.

Actual:

An infinite loop of prompt characters is triggered

Issues/PRs references

Fixes #9946

PeterKietzmann · 2018-10-04T12:08:08Z

@mroethke would you like to give this PR a try?

mroethke · 2018-10-05T09:02:18Z

I can confirm that this fixes my issue.

x3ro · 2018-10-05T09:51:19Z

@PeterKietzmann would you be in for a quick review? ^_^

PeterKietzmann · 2018-10-05T14:07:08Z

ACK from my side. I've added related labels. Out of my head I don't know if there is any other/better solution to shut down. Actually I never really looked into the shell implementation.

I'd say let's keep this open for 1-2 days and if no one complains, I'm gonna merge then.

miri64

I'm also fine with this change. In theory any terminal around should catch an EOF (tested with pyterm and serial_aggregator), so this is only relevant for raw native where there is no terminal program around the input. There it works as expected when hitting Ctrl+D

jia200x · 2018-10-08T09:45:00Z

so, can we merge this one? ;)

miri64 · 2018-10-08T09:48:55Z

If someone gives a proper ACK, I guess. Still like to have the opinion of someone more knowledgeable with the inner workings of the shell though. @kaspar030 @OlegHahm?

kaspar030 · 2018-10-08T10:05:15Z

Please no!

Why does the shell turn off the node? That's not good at all, if only for the added dependency.

kaspar030 · 2018-10-08T10:08:02Z

The shell loop should probably exit on reading EOF.

miri64 · 2018-10-08T10:09:54Z

The shell loop should probably exit on reading EOF.

Now that you say it, this makes more sense to me indeed.

x3ro · 2018-10-08T12:28:05Z

@kaspar030 How would you cause this on a real node (i.e. not native)? Note that it is not turned off when a node receives an EOF, but when getchar returns an error value. In such a case, the current behaviour of the shell would be to infinite loop forever.. Also shell_run is noreturn.

miri64 · 2018-10-08T12:30:33Z

@kaspar030 How would you cause this on a real node (i.e. not native)?

The problem is not that it can't (or may not) be caused on a real node, the problem is that the current proposal is pulling in a dependency (pm) that wasn't there before, just so that one platform [edit] and not even a platform intended for production use [/edit] may finish correctly when EOF is signaled.

x3ro · 2018-10-08T12:58:10Z

So should we modify shell_run to return instead? @kaspar030 @miri64

jcarrano · 2018-10-09T16:29:07Z

@miri64

In theory any terminal around should catch an EOF

No, that's an additional functionality. The terminal emulator should pass though EOF. That's why they have escape keys. The extra "magic" added by the terminal script already gave some headaches when trying non-shell code that communicated via UART, so I'd say it is good if EOFs are handled by RIOT in actual hardware too.

@x3ro
Making the shell return has the advantage of being able to easily restart the node (if one chooses to) by sending a CTRL-D.

tcschmidt · 2018-10-19T15:24:19Z

@x3ro any movement towards 2018.10 Release?

x3ro · 2018-10-19T19:20:53Z

Ah I didn't realize this was marked for the release. I'll take another look at it tomorrow.

x3ro · 2018-10-20T07:20:39Z

@miri64 @kaspar030 @jcarrano Does this look better?

kaspar030 · 2018-10-22T09:07:06Z

tests/shell/main.c

    shell_run(NULL, line_buf, SHELL_DEFAULT_BUFSIZE);
    */

+    pm_off();


IMO this is change should get its own PR.

kaspar030 · 2018-10-22T09:07:42Z

sys/include/shell.h

@@ -72,7 +72,7 @@ typedef struct shell_command_t {
 *
 * @returns         This function does not return.


this message needs updating

jia200x · 2018-10-26T08:53:38Z

@x3ro I'm OK with the proposed change in the shell. Could you address @kaspar030 comments?

This bugfix is listed in the Release 2018.10, so would be very nice to have it :)

jia200x · 2018-10-30T09:09:30Z

ping @x3ro

x3ro · 2018-10-30T16:44:24Z

@jia200x want to give it another look?

jia200x · 2018-10-30T18:04:44Z

@kaspar030 comments were addressed. Re-tested with current master and with this PR, and works as expected.

jia200x

Tested ACK

In RIOT native, sending CTRL+D to a shell started using shell_run would resulted in and endless prompt loop. I've been unable to trigger such a behaviour on actual hardware using a UART connection, but calling `pm_off` seemed like a better alternative than having an `#ifdef BOARD_NATIVE`. Fixes #9946

cladmi · 2018-10-30T19:08:46Z

I just saw this one in master, and the commit message is not prepended by the module/file and the commit message does not match the implemented fix.
Maybe it was not ready for merging ?

cladmi · 2018-10-30T19:10:03Z

It was also an API change in theory so would have waited for another ack I think.

jia200x · 2018-10-30T19:18:30Z

I just saw this one in master, and the commit message is not prepended by the module/file and the commit > message does not match the implemented fix.
Maybe it was not ready for merging ?

I wrongly ACK'd. Sorry.

x3ro · 2018-10-30T20:30:03Z

@cladmi Apologies for missing the commit message, I was under the impression that Murdock checks this, so I didn't pay much attention after squashing.

It was also an API change in theory so would have waited for another ack I think.

Nobody had any issues with the API change so far, and the issue has been open for a month. The changes today were purely removing things that reviewers did not feel like it belonged in the PR. From the API change side, it feels fairly well reviewed IMO.

cladmi · 2018-10-31T11:21:34Z

@x3ro to prevent forgetting to update messages, I tend to do my fix with git commit --squash HASH with the updated message in the body. This way when squashing with --autosquash I am asked to squash it and I remember to update the first message.

For the API change, the thing is that it should normally have 2 maintainers ACKs, and should not be backported after the feature-freeze. Even more when this is for a native specific native ease of life improvement.
The change is great, but I would not say worth changing the API during the release RC.

x3ro force-pushed the fix-shell-prompt-infinite-loop branch from 4622f85 to f3e781f Compare October 2, 2018 16:19

x3ro added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: native Platform: This PR/issue effects the native platform labels Oct 2, 2018

miri64 reviewed Oct 5, 2018

View reviewed changes

jia200x added this to the Release 2018.10 milestone Oct 8, 2018

kaspar030 reviewed Oct 22, 2018

View reviewed changes

jia200x modified the milestones: Release 2018.10, Release 2019.01 Oct 24, 2018

jia200x added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 30, 2018

jia200x approved these changes Oct 30, 2018

View reviewed changes

x3ro force-pushed the fix-shell-prompt-infinite-loop branch from 7da6d09 to 62cecc9 Compare October 30, 2018 18:07

x3ro merged commit 6ed11de into RIOT-OS:master Oct 30, 2018

x3ro mentioned this pull request Oct 30, 2018

Do not try to continue reading shell commands if input source is closed #10304

Closed

jia200x removed this from the Release 2019.01 milestone Nov 1, 2018

jia200x removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Nov 1, 2018

jia200x mentioned this pull request Nov 6, 2018

Release 2018.10 notes #10257

Merged

jcarrano mentioned this pull request Jan 16, 2019

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

Closed

fedepell mentioned this pull request Feb 22, 2019

sys/shell: handle correctly long strings that overflow buffer #11054

Closed

		@@ -72,7 +72,7 @@ typedef struct shell_command_t {
		*
		* @returns This function does not return.

Conversation

x3ro commented Oct 2, 2018

Contribution description

Testing procedure

Expected:

Actual:

Issues/PRs references

Uh oh!

PeterKietzmann commented Oct 4, 2018

Uh oh!

mroethke commented Oct 5, 2018

Uh oh!

x3ro commented Oct 5, 2018

Uh oh!

PeterKietzmann commented Oct 5, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Oct 8, 2018

Uh oh!

miri64 commented Oct 8, 2018

Uh oh!

kaspar030 commented Oct 8, 2018

Uh oh!

kaspar030 commented Oct 8, 2018

Uh oh!

miri64 commented Oct 8, 2018

Uh oh!

x3ro commented Oct 8, 2018

Uh oh!

miri64 commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

x3ro commented Oct 8, 2018

Uh oh!

jcarrano commented Oct 9, 2018

Uh oh!

tcschmidt commented Oct 19, 2018

Uh oh!

x3ro commented Oct 19, 2018

Uh oh!

x3ro commented Oct 20, 2018

Uh oh!

kaspar030 Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

jcarrano Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

kaspar030 Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x commented Oct 26, 2018

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

x3ro commented Oct 30, 2018

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Oct 30, 2018

Uh oh!

cladmi commented Oct 30, 2018

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

x3ro commented Oct 30, 2018

Uh oh!

cladmi commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

miri64 commented Oct 8, 2018 •

edited

Loading