sys/shell: refactor tokenizer code #13197

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

Contribution description

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

The tokenizer (the code that breaks up the line given to the shell into strings to create argv) was quite a messy piece of code. This commit refactors it into a more traditional state-machine based parser.

This fixes the issues with quote handling exposed by the recently introduced test.

Testing procedure

Test vectors were added to the automated test. The old code could not handle this string:

> echo "t\e st" "\"" '\'' a\ b
shell: incorrect quoting
> echo "\""
shell: incorrect quoting

The new code handles it correctly.

Issues/PRs references

Copy of #12085

Dependencies:

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

sys/shell/shell.c

HendrikVE · 2020-03-30T17:10:38Z

Rebased and ready for review :)

Check that single and double quotes work, along with backslash escaping and that malformed strings are rejected. Right now the test is failing. The next commit will replace the tokenizer with one that works correctly. Co-authored-by: Juan Carrano <[email protected]>

The tokenizer (the code that breaks up the line given to the shell into strings to create argv) was quite a messy piece of code. This commit refactors it into a more traditional state-machine based parser. This fixes the issues with quote handling exposed by the recently introduced test. Co-authored-by: Juan Carrano <[email protected]>

HendrikVE · 2020-05-18T13:15:04Z

Rebased to master and Murdock builds were successful.

HendrikVE · 2020-06-08T11:56:27Z

Could you please also take a look at this one, @fjmolinas ? :)

HendrikVE · 2020-06-19T11:57:58Z

Tabs are processed as before now. I replaced the state diagram by a text description which I hope is clear enough. I also did some other improvements in the code.

fjmolinas · 2020-06-19T15:14:21Z

For the record, code size:

pr
master

fjmolinas · 2020-06-19T15:17:44Z

I'll take a closer look at the latest changes on Monday!

fjmolinas

Two small nitpicks, otherwise this PR is OK IMO. I'll run the test on many boards and see if everything is ok.

fjmolinas · 2020-06-23T07:50:18Z

sys/shell/shell.c

+/**
+ * Break input line into words, create argv and call the command handler.
+ *
+ * Words are broken up at spaces. A backslash escaped the character that comes


Suggested change

* Words are broken up at spaces. A backslash escaped the character that comes

* Words are broken up at spaces. A backslash escapes the character that comes

fjmolinas · 2020-06-23T07:53:07Z

tests/shell/tests/01-run.py

+    ('echo abcdef"ghijklmn', 'shell: incorrect quoting'),
+    ('echo abcdef\'ghijklmn', 'shell: incorrect quoting'),
+
+    # test more complex output


Maybe:

Suggested change

# test more complex output

# test default commands

fjmolinas · 2020-06-23T10:26:36Z

All good from my local tests https://ci.inria.fr/ci-riot-tribe/job/build-pipeline.jk/lastSuccessfulBuild/artifact/failuresummary.md/*view*/ (the failing boards had connection issues, I re-launched the test and it worked).

fjmolinas · 2020-06-23T10:26:54Z

@HendrikVE please squash once my last remaining comments are addressed!

Factor out common code for quoted and unquoted tokens. This makes the code slighly less clear, but it also eliminates repetition (which may improve clarity). Co-authored-by: Juan Carrano <[email protected]>

The code for traversing arrays of shell commands (used to print help messages and to search for commmand handlers) was needlessly complex. Co-authored-by: Juan Carrano <[email protected]>

Code now correctly handles quotes within PARSE_UNQUOTED and tabs are now considered a BLANK just like a space.

Divide test cases in to groups and add test cases for: - multiple spaces between arguments - tabs between arguments - leading/trailing spaces - more simple variations for escaping - multiple tests for correct quoting A second occurence of the test case "('help', EXPECTED_HELP)," was removed.

HendrikVE · 2020-06-23T12:01:07Z

I changed the comments as suggested and squashed my commits. But this time, I'm not sure about how much they should be squashed. One possibility would be to have 2 commits sys/shell and test/shell, but then the size of the commit would be quite big. What would you suggest here?

fjmolinas · 2020-06-23T12:47:35Z

I changed the comments as suggested and squashed my commits. But this time, I'm not sure about how much they should be squashed. One possibility would be to have 2 commits sys/shell and test/shell, but then the size of the commit would be quite big. What would you suggest here?

I prefer keeping the division as you have now, I'd rather split up the different changes. So LGTM!

fjmolinas · 2020-06-23T12:47:44Z

Lets see if murdock is happy.

fjmolinas

ACK, the code changes improve:

readability
escaped characters handling
quoted strings handling

There is an increase in the shell code, but the code itself is pretty complex, so IMO that change is expected. Maybe some people obsessed by code size want to take a second look (@kaspar030 @bergzand ), but IMO this is OK. ACK!

fjmolinas · 2020-06-23T14:41:18Z

Ajj seems like there is a problem with a worker again...

fjmolinas · 2020-06-24T10:32:47Z

Go!

HendrikVE · 2020-06-24T12:23:20Z

Awesome, thank you @fjmolinas !

HendrikVE mentioned this pull request Jan 24, 2020

sys/shell: refactor tokenizer code. #12085

Closed

benpicco added Area: sys Area: System Type: cleanup The issue proposes a clean-up / The PR cleans-up 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 25, 2020

HendrikVE mentioned this pull request Feb 7, 2020

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

Merged

HendrikVE added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 7, 2020

HendrikVE force-pushed the shell-tokenizer-refactor branch 3 times, most recently from d956584 to 3d08898 Compare February 10, 2020 12:24

HendrikVE added Area: tests Area: tests and testing framework Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Feb 10, 2020

HendrikVE mentioned this pull request Mar 30, 2020

sys/shell: refactor readline function #13196

Merged

1 task

HendrikVE removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 30, 2020

HendrikVE force-pushed the shell-tokenizer-refactor branch from 3d08898 to bf6485c Compare March 30, 2020 14:33

HendrikVE requested a review from miri64 as a code owner March 30, 2020 14:33

HendrikVE removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 30, 2020

HendrikVE commented Mar 30, 2020

View reviewed changes

sys/shell/shell.c Show resolved Hide resolved

HendrikVE force-pushed the shell-tokenizer-refactor branch 2 times, most recently from 5fec2d5 to a61d66a Compare March 30, 2020 16:24

HendrikVE and others added 2 commits May 18, 2020 14:02

HendrikVE force-pushed the shell-tokenizer-refactor branch from a61d66a to ee1e1ad Compare May 18, 2020 12:34

fjmolinas requested review from fjmolinas and kaspar030 June 8, 2020 12:19

fjmolinas added the CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before label Jun 9, 2020

HendrikVE force-pushed the shell-tokenizer-refactor branch from 9efd64e to ad7f6c4 Compare June 19, 2020 13:59

fjmolinas reviewed Jun 23, 2020

View reviewed changes

HendrikVE and others added 4 commits June 23, 2020 13:46

sys/shell: further refactor tokenizer (part 1/2)

37cff93

Factor out common code for quoted and unquoted tokens. This makes the code slighly less clear, but it also eliminates repetition (which may improve clarity). Co-authored-by: Juan Carrano <[email protected]>

sys/shell: simplify array traversal code

0782b49

The code for traversing arrays of shell commands (used to print help messages and to search for commmand handlers) was needlessly complex. Co-authored-by: Juan Carrano <[email protected]>

sys/shell: further refactor tokenizer (part 2/2)

cc759eb

Code now correctly handles quotes within PARSE_UNQUOTED and tabs are now considered a BLANK just like a space.

HendrikVE force-pushed the shell-tokenizer-refactor branch from 6f12972 to cbf5cc5 Compare June 23, 2020 11:58

fjmolinas approved these changes Jun 23, 2020

View reviewed changes

fjmolinas mentioned this pull request Jun 24, 2020

core: make idle thread optional #14224

Merged

fjmolinas merged commit af80e86 into RIOT-OS:master Jun 24, 2020

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

HendrikVE deleted the shell-tokenizer-refactor branch June 26, 2020 09:35

miri64 mentioned this pull request Jul 1, 2020

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/shell: refactor tokenizer code #13197

sys/shell: refactor tokenizer code #13197

HendrikVE commented Jan 24, 2020 •

edited

Loading

HendrikVE commented Mar 30, 2020

HendrikVE commented May 18, 2020

HendrikVE commented Jun 8, 2020

HendrikVE commented Jun 19, 2020

fjmolinas commented Jun 19, 2020

fjmolinas commented Jun 19, 2020

fjmolinas left a comment

fjmolinas Jun 23, 2020

fjmolinas Jun 23, 2020

fjmolinas commented Jun 23, 2020

fjmolinas commented Jun 23, 2020

HendrikVE commented Jun 23, 2020 •

edited

Loading

fjmolinas commented Jun 23, 2020

fjmolinas commented Jun 23, 2020

fjmolinas left a comment

fjmolinas commented Jun 23, 2020

fjmolinas commented Jun 24, 2020

HendrikVE commented Jun 24, 2020

	* Words are broken up at spaces. A backslash escaped the character that comes
	* Words are broken up at spaces. A backslash escapes the character that comes

sys/shell: refactor tokenizer code #13197

sys/shell: refactor tokenizer code #13197

Conversation

HendrikVE commented Jan 24, 2020 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

Dependencies:

HendrikVE commented Mar 30, 2020

HendrikVE commented May 18, 2020

HendrikVE commented Jun 8, 2020

HendrikVE commented Jun 19, 2020

fjmolinas commented Jun 19, 2020

fjmolinas commented Jun 19, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas Jun 23, 2020

Choose a reason for hiding this comment

fjmolinas Jun 23, 2020

Choose a reason for hiding this comment

fjmolinas commented Jun 23, 2020

fjmolinas commented Jun 23, 2020

HendrikVE commented Jun 23, 2020 • edited Loading

fjmolinas commented Jun 23, 2020

fjmolinas commented Jun 23, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas commented Jun 23, 2020

fjmolinas commented Jun 24, 2020

HendrikVE commented Jun 24, 2020

HendrikVE commented Jan 24, 2020 •

edited

Loading

HendrikVE commented Jun 23, 2020 •

edited

Loading