[riot-notifications] [RIOT-OS/RIOT] sys/shell: correctly detect and handle long lines (#10635)

Juan I Carrano notifications at github.com
Wed Jan 16 15:26:11 CET 2019


jcarrano commented on this pull request.

Added some comments on the reviews that dissapeared because of GH magic.

> @@ -280,21 +320,31 @@ static inline void print_prompt(void)
 #endif
 }
 
+static const char *TOOLONG_MESSAGE = "shell: maximum line length exceeded";

```suggestion
#define TOOLONG_MESSAGE "shell: maximum line length exceeded"
```

> + * stdout and also supports primitive line editing.
+ *
+ * If the input line is too long, the input will still be consumed until the end
+ * to prevent the next line from containing garbage.
+ *
+ * @param   buf     Buffer where the input will be placed.
+ * @param   size    Size of the buffer. The maximum line length will be one less
+ *                  than size, to accommodate for the null terminator.
+ *                  The minimum buffer size is 1.
+ *
+ * @return  length of the read line, excluding the terminator, if reading was
+ *          successful.
+ * @return  EOF, if the if the end of the input stream is reached.
+ * @return  -READLINE_TOOLONG if the buffer size was exceeded
+ */
+static ssize_t readline(char *buf, size_t size)

> How about we change the signature of the function to "size_t".
> Empty line returns "0". Line with text returns it's length.
> For the error cases, we define an enum, or re-use something from errno. ENOBUFS, EBADFD come to mind.

I did more or less that, but avoided errno, and if the line is tool long it returns an error, so that the length checking logic does not have to be in the main shell loop.

> @@ -260,7 +295,13 @@ static int readline(char *buf, size_t size)
 #endif
         }
         else {
-            *line_buf_ptr++ = c;

> Would you mind defining a "buf_end" ptr? char *buf_end = buf + size, and use it here?

Pointer manipulation is gone, replaced by indices. Turns out that once one has to keep track of lengths the code becomes cleaner with explicit indices.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10635#pullrequestreview-193155878
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190116/706aafbb/attachment.html>


More information about the notifications mailing list