shell: Use argc and argv in shell handlers by Kijewski · Pull Request #760 · RIOT-OS/RIOT

Kijewski · 2014-02-21T18:18:01Z

Compare #708.

Now the tokenization of an input line is done by the shell itself. You
may quote arguments with "...". Empty arguments, supplied by "" are
preserved. Spaces in between arguments are squasheds; spaces inside
quotes are preserved.

You cannot partially quote an argument. You must not use

cmd "abc,
cmd abc"def", or
cmd "abc"def.

LudwigKnuepfer · 2014-02-21T18:19:48Z

What happens if I do?

Kijewski · 2014-02-21T18:23:23Z

The first prints a warning and does not call the handler.
The second supplies abc"def" as one argument, without touching the quotes.
The third is parsed like cmd abc def (two arguments).

I added the command echo to test_shell. It prints the tokenized arguments.

LudwigKnuepfer · 2014-02-21T18:28:55Z

Personally I would prefer if the second one would drop the trailing space and the third would not be chopped up. Do you think that's feasible? (I would have gone for a state machine .. haven't looked into your implementation, just telling ;-)

LudwigKnuepfer · 2014-02-21T18:30:48Z

Pfft... state machine (what is that supposed to mean?!).. I need to rest!

Kijewski · 2014-02-21T18:42:06Z

Now all three kinds of wrong quoting cause an error message.
I guess that's the best approach.

kaspar030 · 2014-02-21T20:15:18Z

sys/shell/shell.c

The comment does not match the line that immediately follows it?

Ahh, I did not put a newline after this line, so it's supposed to be the same logical block.
But now I see that @kaspar030 probably meant shell_command_handler_t handler = handler = ....

mehlis · 2014-02-24T08:38:56Z

@Kijewski nice work, but you missed all the apps in the example folder

Kijewski · 2014-02-24T22:20:13Z

@mehlis I don't think that I understand riot_ccn_register_prefix in ccn-line-client.
You should be able to omit the first argument, but the following two are obligatory?

Kijewski · 2014-02-24T22:44:27Z

examples/rpl_udp/udp.c

text was and is declared as char[5]. The previous implementation did not test the input, so I don't know if the limitation is needed at all.

LudwigKnuepfer · 2014-02-25T06:37:16Z

OK, only one thing missing:

> echo 1 2 3
“echo” “1” “2” “3” 
> echo 
“echo” “1” “2” “3”

The second invocation should not reuse the old parameters.

mehlis · 2014-02-25T10:25:35Z

@Kijewski PR needs rebase

Kijewski · 2014-02-25T16:32:43Z

I didn't convert, riot_ccn_register_prefix, yet. I don't understand the function.
Maybe @mehlis could convert this function and open a PR in my repo? :)

Compare #708. Now the tokenization of an input line is done by the shell itself. You may quote arguments with `"..."`. Empty arguments, supplied by `""` are preserved. Spaces in between arguments are squasheds; spaces inside quotes are preserved. You cannot partially quote an argument. You must not use - `cmd "abc`, - `cmd abc"def"`, or - `cmd "abc"def`.

Kijewski · 2014-02-25T16:58:28Z

Rebased.

example/ccn-lite-client waits for @mehlis' input
example/rpl_udp does not build for master either …

mehlis · 2014-02-26T08:11:51Z

@Kijewski I'll provide an update for my code.

mehlis · 2014-02-26T08:12:48Z

@Kijewski rpl_udp@master is compiling in my system, or do you mean with this PR it is not compiling anymore?

LudwigKnuepfer · 2014-02-26T08:40:57Z

regarding #760 (comment):
You could just memset the whole buffer after the command handler returns.

--- a/sys/shell/shell.c
+++ b/sys/shell/shell.c
@@ -92,6 +92,8 @@ static void handle_input_line(shell_t *shell, char *line)
 {
     static const char *INCORRECT_QUOTING = "shell: incorrect quoting";

+    size_t linelen = strlen(line);
+
     /* first we need to calculate the number of arguments */
     unsigned argc = 0;
     char *pos;
@@ -160,6 +162,7 @@ static void handle_input_line(shell_t *shell, char *line)
             puts("shell: command not found.");
         }
     }
+    memset(line, 0, linelen);
 }

LudwigKnuepfer · 2014-02-26T08:44:40Z

Maybe this belongs in the uart handler though...

LudwigKnuepfer · 2014-02-26T08:51:55Z

--- a/sys/shell/shell.c
+++ b/sys/shell/shell.c
@@ -208,6 +208,7 @@ void shell_run(shell_t *shell)

         if (!res) {
             handle_input_line(shell, line_buf);
+            memset(line_buf, 0, shell->shell_buffer_size);
         }

         print_prompt(shell);

Kijewski · 2014-02-26T12:08:24Z

There is really a bug in my implementation. The input line only has to be null terminated, not zeroed out. I'll look into it.

haukepetersen · 2014-02-26T12:19:56Z

I tried out Ludwigs fixes for preventing old parameters to show up, I would go for the memset in handle_input_line(), as it is more efficient as not the entire buffer is set every time...

Kijewski · 2014-02-26T12:22:58Z

Terminating the string with two zeros would have worked, but I changed the implementation a bit in my last commit.

LudwigKnuepfer · 2014-02-26T12:25:55Z

It is still broken though. Also - I think the buffer need to be zeroed out at least once before using it.

LudwigKnuepfer · 2014-02-26T12:27:49Z

--- a/sys/shell/shell.c
+++ b/sys/shell/shell.c
@@ -200,6 +200,7 @@ static inline void print_prompt(shell_t *shell)
 void shell_run(shell_t *shell)
 {
     char line_buf[shell->shell_buffer_size];
+    memset(line_buf, 0, shell->shell_buffer_size);

     print_prompt(shell);

LudwigKnuepfer · 2014-02-26T12:28:06Z

valgrind is of my opinion here..

Kijewski · 2014-02-26T12:31:21Z

$ rm -r ./bin
$ make BOARD=native all-valgrind
$ valgrind ./bin/native/*.elf
==10739== Memcheck, a memory error detector
==10739== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==10739== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==10739== Command: ./bin/native/test_shell.elf
==10739== 
RIOT native uart0 initialized.
RIOT native interrupts/signals initialized.
LED_GREEN_OFF
LED_RED_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

kernel_init(): This is RIOT! (Version: 2014.01-333-g28c4-localhost-shell-args)
kernel_init(): jumping into first task...
test_shell.
UART0 thread started.
uart0_init() [OK]
> echo 1 2 3
echo 1 2 3
“echo” “1” “2” “3” 
> echo
echo
“echo” 
> ./bin/native/test_shell.elf: closed stdio
^Clpm_set(): exit()
==10739== 
==10739== HEAP SUMMARY:
==10739==     in use at exit: 0 bytes in 0 blocks
==10739==   total heap usage: 7 allocs, 7 frees, 700 bytes allocated
==10739== 
==10739== All heap blocks were freed -- no leaks are possible
==10739== 
==10739== For counts of detected and suppressed errors, rerun with: -v
==10739== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

LudwigKnuepfer · 2014-02-26T12:32:41Z

Try echo without parameters.

Kijewski · 2014-02-26T12:33:42Z

$ valgrind ./bin/native/*.elf
==10778== Memcheck, a memory error detector
==10778== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==10778== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==10778== Command: ./bin/native/test_shell.elf
==10778== 
RIOT native uart0 initialized.
RIOT native interrupts/signals initialized.
LED_GREEN_OFF
LED_RED_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

kernel_init(): This is RIOT! (Version: 2014.01-333-g28c4-localhost-shell-args)
kernel_init(): jumping into first task...
test_shell.
UART0 thread started.
uart0_init() [OK]
> echo
echo
“echo” 
> ./bin/native/test_shell.elf: closed stdio
^Clpm_set(): exit()
==10778== 
==10778== HEAP SUMMARY:
==10778==     in use at exit: 0 bytes in 0 blocks
==10778==   total heap usage: 3 allocs, 3 frees, 300 bytes allocated
==10778== 
==10778== All heap blocks were freed -- no leaks are possible
==10778== 
==10778== For counts of detected and suppressed errors, rerun with: -v
==10778== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Did you fetch my last commit https://github.com/Kijewski/RIOT/commit/28c4ef45f703ea5a9fd3545abd22c9cdeed40a6c?

LudwigKnuepfer · 2014-02-26T12:34:33Z

Ah, very well - I think I fucked up when updating my copy of your branch.

LudwigKnuepfer · 2014-02-26T12:35:32Z

Yup, everything is fine now.

haukepetersen · 2014-02-26T12:38:11Z

As far as I'm concerned this PR is quite useful and it's running fine on the MSBA2 and the avsextrem, so I give it an ACK

LudwigKnuepfer · 2014-02-26T12:40:00Z

sc_rtc.c is broken.

Kijewski · 2014-02-26T12:40:29Z

I think it's important that someone reviews my changes to the actual shell handlers. Most of them were trivial, but there were monsters like sc_disk.c, too.

Kijewski · 2014-02-26T12:48:28Z

@LudwigOrtmann, I admit, I'm a noob. :) Could you give me some pointers how to use the default example?

LudwigKnuepfer · 2014-02-26T12:51:28Z

rtc has online help ;-)
Anyways - date 0000-00-00 00:00:00 this has two parameters, the handler only passes one on.

LudwigKnuepfer · 2014-02-26T12:52:15Z

Or what were you referring to?

mehlis · 2014-02-26T12:56:50Z

@Kijewski PR for register prefix is open (on your branch)

mehlis · 2014-02-26T13:02:27Z

tested with ccn-lite-client with network, ok for me

shell: adapt register prefix command to new shell api

Kijewski · 2014-02-26T13:19:45Z

I fixed my errors in sc_rtc and merged @mehlis' PR.

mehlis · 2014-02-26T13:43:20Z

@LudwigOrtmann any other findings?

from my side: ACK

LudwigKnuepfer · 2014-02-26T13:48:29Z

@Kijewski regarding sc_disk:
Feel free to compare here:
https://github.com/LudwigOrtmann/RIOT/commits/fix_shell_strtok

As there is no code to test this with at the moment I don't really care if it works or not though ;-)

LudwigKnuepfer · 2014-02-26T13:56:45Z

ACK - "There can be nothing wrong in this PR that isn't outweighed with what is right in this PR".

shell: Use argc and argv in shell handlers

Kijewski mentioned this pull request Feb 21, 2014

Change Shell Handler Signature Completely #708

Closed

LudwigKnuepfer added enhancement labels Feb 21, 2014

kaspar030 reviewed Feb 21, 2014
View reviewed changes

Kijewski reviewed Feb 24, 2014
View reviewed changes

OlegHahm removed the PR-award-nominee label Feb 24, 2014

Kijewski added 3 commits February 25, 2014 17:54

Print error message on any quoting error

829966e

Convert examples

0eb7597

Do not skip over the null terminator

28c4ef4

shell: adapt register prefix command to new shell api

9d9f114

Kijewski and others added 2 commits February 26, 2014 14:08

Fix _settime_handler

a5c0518

Merge pull request #3 from mehlis/shell-args-prefix

5cab0ea

shell: adapt register prefix command to new shell api

mehlis pushed a commit that referenced this pull request Feb 26, 2014

Merge pull request #760 from Kijewski/shell-args

92eaa51

shell: Use argc and argv in shell handlers

mehlis merged commit 92eaa51 into RIOT-OS:master Feb 26, 2014

Kijewski deleted the shell-args branch February 26, 2014 14:38

LudwigKnuepfer mentioned this pull request Feb 26, 2014

sys: shell: what's wrong #742

Closed

OlegHahm added the PR-award-nominee label Feb 26, 2014

Enoch247 mentioned this pull request Dec 21, 2023

sys/shell: fix getopt() support #20209

Merged

Conversation

Kijewski commented Feb 21, 2014

Uh oh!

LudwigKnuepfer commented Feb 21, 2014

Uh oh!

Kijewski commented Feb 21, 2014

Uh oh!

LudwigKnuepfer commented Feb 21, 2014

Uh oh!

LudwigKnuepfer commented Feb 21, 2014

Uh oh!

Kijewski commented Feb 21, 2014

Uh oh!

kaspar030 Feb 21, 2014

Choose a reason for hiding this comment

Uh oh!

Kijewski Feb 22, 2014

Choose a reason for hiding this comment

Uh oh!

LudwigKnuepfer Feb 22, 2014

Choose a reason for hiding this comment

Uh oh!

Kijewski Feb 22, 2014

Choose a reason for hiding this comment

Uh oh!

LudwigKnuepfer Feb 22, 2014

Choose a reason for hiding this comment

Uh oh!

mehlis commented Feb 24, 2014

Uh oh!

Kijewski commented Feb 24, 2014

Uh oh!

Kijewski Feb 24, 2014

Choose a reason for hiding this comment

Uh oh!

LudwigKnuepfer commented Feb 25, 2014

Uh oh!

mehlis commented Feb 25, 2014

Uh oh!

Kijewski commented Feb 25, 2014

Uh oh!

Kijewski commented Feb 25, 2014

Uh oh!

mehlis commented Feb 26, 2014

Uh oh!

mehlis commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

Kijewski commented Feb 26, 2014

Uh oh!

haukepetersen commented Feb 26, 2014

Uh oh!

Kijewski commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

Kijewski commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

Kijewski commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

haukepetersen commented Feb 26, 2014

Uh oh!

LudwigKnuepfer commented Feb 26, 2014

Uh oh!

Kijewski commented Feb 26, 2014