[PATCH rtems6 1/1] libmisc/shell: Fix timeout in getting terminal size
Gedare Bloom
gedare at rtems.org
Mon Jan 22 17:16:05 UTC 2024
I have a couple minor notes below. More important, does this change
require updating documentation?
I know we have a somewhat aging shell-specific guide:
https://docs.rtems.org/branches/master/shell/index.html
On Fri, Jan 19, 2024 at 5:19 AM <dufault at hda.com> wrote:
>
> From: Peter Dufault <dufault at hda.com>
>
> - Fix detection of timeout in rtems_shell_term_wait_for().
>
> - Switch to using "termios" VTIME inter-character timeout.
> The previous implementation is dependent on the BSP clock tick value.
>
> Updates #4763
> ---
> cpukit/libmisc/shell/shell.c | 101 ++++++++++++++++++++++++++-----------------
> 1 file changed, 62 insertions(+), 39 deletions(-)
>
> diff --git a/cpukit/libmisc/shell/shell.c b/cpukit/libmisc/shell/shell.c
> index 9cefc80..60cdb4f 100644
> --- a/cpukit/libmisc/shell/shell.c
> +++ b/cpukit/libmisc/shell/shell.c
> @@ -805,28 +805,52 @@ void rtems_shell_print_env(
> }
> #endif
>
> +/* Debugging output for the terminal I/O associated with window size detection
> + * can be sent with rtems_shell_winsize_db().
> + */
> +
> +/* Window size detection knobs.
> + */
> +static bool rtems_shell_winsize_dbf; /* Debug. "true" to debug. */
Is this necessary? How does the application set/test this?
> +static int rtems_shell_winsize_vtime = 1; /* VTIME timeout in .1 seconds */
> +
> +static void rtems_shell_winsize_vdb(const char *format, va_list ap) {
> + if (rtems_shell_winsize_dbf == false)
> + return;
> + printf("shell_winsize: ");
> + vprintf(format, ap);
> + fflush(stdout);
> +}
> +
> +static void rtems_shell_winsize_db(const char *format, ...) {
> + va_list ap;
> + va_start(ap, format);
> + rtems_shell_winsize_vdb(format, ap);
> + va_end(ap);
> +}
> +
> /*
> * Wait for the string to return or timeout.
> */
> -static bool rtems_shell_term_wait_for(const int fd, const char* str, const int timeout)
> +static bool rtems_shell_term_wait_for(const int fd, const char* str)
> {
> - int msec = timeout;
> int i = 0;
> - while (msec-- > 0 && str[i] != '\0') {
> + while (str[i] != '\0') {
> char ch[2];
> - if (read(fd, &ch[0], 1) == 1) {
> - fflush(stdout);
> + ssize_t n;
> + if ((n = read(fd, &ch[0], 1)) == 1) {
> if (ch[0] != str[i++]) {
> + rtems_shell_winsize_db("Wrong char at char %d.\n", i);
> return false;
> }
> - msec = timeout;
> } else {
> - usleep(1000);
> + rtems_shell_winsize_db("%s reading char %d.\n",
> + (n == 0) ? "Timeout" : "Error", i);
I'd suggest putting the 'i' on it's own line here. I don't know that
there's a specific style guide for this file, but having trailing
statements after the ternary is a bit harder to read. imo
> + return false;
> }
> }
> - if (msec == 0) {
> - return false;
> - }
> +
> + rtems_shell_winsize_db("Matched string.\n");
> return true;
> }
>
> @@ -836,41 +860,42 @@ static bool rtems_shell_term_wait_for(const int fd, const char* str, const int t
> static int rtems_shell_term_buffer_until(const int fd,
> char* buf,
> const int size,
> - const char* end,
> - const int timeout)
> + const char* end)
> {
> - int msec = timeout;
> int i = 0;
> int e = 0;
> memset(&buf[0], 0, size);
> - while (msec-- > 0 && i < size && end[e] != '\0') {
> + while (i < size && end[e] != '\0') {
> char ch[2];
> - if (read(fd, &ch[0], 1) == 1) {
> - fflush(stdout);
> + ssize_t n;
> + if ( (n = read(fd, &ch[0], 1)) == 1) {
> buf[i++] = ch[0];
> if (ch[0] == end[e]) {
> e++;
> } else {
> + rtems_shell_winsize_db("Reset search for end string.\n");
> e = 0;
> }
> - msec = timeout;
> } else {
> - usleep(1000);
> + /* Error or timeout. */
> + rtems_shell_winsize_db(
> + "%s looking for end string \"%s\". Read \"%s\".\n",
> + (n == 0) ? "Timeout" : "Error", end, buf
> + );
> + return -1;
> }
> }
> - if (msec == 0 || end[e] != '\0') {
> - return -1;
> - }
> - i -= e;
> + i -= e; /* Toss away the matched end. */
> if (i < size) {
> buf[i] = '\0';
> }
> + rtems_shell_winsize_db("Found end string \"%s\".\n", end);
> return i;
> }
>
> /*
> - * Determine if the terminal has the row and column values
> - * swapped
> + * Determine if this is a version of the "tmux" terminal emulator with
> + * the row and column values swapped
> *
> * https://github.com/tmux/tmux/issues/3457
> *
> @@ -878,19 +903,19 @@ static int rtems_shell_term_buffer_until(const int fd,
> * in the time it takes to get the fix into code so see if tmux is
> * running and which version and work around the bug.
> *
> - * The terminal device needs to have VMIN=0, and VTIME=0
> + * The terminal device needs to have VMIN=0, and VTIME set to the
> + * desired timeout in .1 seconds.
> */
> -static bool rtems_shell_term_row_column_swapped(const int fd, const int timeout) {
> +static bool rtems_shell_term_row_column_swapped(const int fd, const int fout) {
> char buf[64];
> memset(&buf[0], 0, sizeof(buf));
> /*
> * CSI > Ps q
> * Ps = 0 => DCS > | text ST
> */
> - fputs("\033[>0q", stdout);
> - fflush(stdout);
> - if (rtems_shell_term_wait_for(fd, "\033P>|", timeout)) {
> - int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "\033\\", timeout);
> + write(fout, "\033[>0q", 5);
> + if (rtems_shell_term_wait_for(fd, "\033P>|")) {
> + int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "\033\\");
> if (len > 0) {
> if (memcmp(buf, "tmux ", 5) == 0) {
> static const char* bad_versions[] = {
> @@ -916,8 +941,8 @@ static bool rtems_shell_term_row_column_swapped(const int fd, const int timeout)
> static void rtems_shell_winsize( void )
> {
> const int fd = fileno(stdin);
> + const int fout = fileno(stdout);
> struct winsize ws;
> - const int timeout = 150;
> char buf[64];
> bool ok = false;
> int lines = 0;
> @@ -933,18 +958,16 @@ static void rtems_shell_winsize( void )
> if (tcgetattr(fd, &cterm) >= 0) {
> struct termios term = cterm;
> term.c_cc[VMIN] = 0;
> - term.c_cc[VTIME] = 0;
> - if (tcsetattr (fd, TCSADRAIN, &term) >= 0) {
> + term.c_cc[VTIME] = rtems_shell_winsize_vtime;
> + if (tcsetattr (fd, TCSAFLUSH, &term) >= 0) {
> memset(&buf[0], 0, sizeof(buf));
> /*
> * https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Miscellaneous
> *
> * CSI 1 8 t
> */
> - fputs("\033[18t", stdout);
> - fflush(stdout);
> - if (rtems_shell_term_wait_for(fd, "\033[8;", timeout)) {
> - int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), ";", timeout);
> + if (write(fout, "\033[18t", 5) == 5 && rtems_shell_term_wait_for(fd, "\033[8;")) {
> + int len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), ";");
> if (len > 0) {
> int i;
> lines = 0;
> @@ -953,7 +976,7 @@ static void rtems_shell_winsize( void )
> lines *= 10;
> lines += buf[i++] - '0';
> }
> - len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "t", timeout);
> + len = rtems_shell_term_buffer_until(fd, buf, sizeof(buf), "t");
> if (len > 0) {
> cols = 0;
> i = 0;
> @@ -966,7 +989,7 @@ static void rtems_shell_winsize( void )
> }
> }
> }
> - if (rtems_shell_term_row_column_swapped(fd, timeout)) {
> + if (ok && rtems_shell_term_row_column_swapped(fd, fout)) {
> int tmp = lines;
> lines = cols;
> cols = tmp;
> --
> 1.8.3.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list