[PATCH rtems6 1/1] libmisc/shell: Fix timeout in getting terminal size
Peter Dufault
dufault at hda.com
Mon Jan 22 18:51:56 UTC 2024
> On Jan 22, 2024, at 12:16 PM, Gedare Bloom <gedare at rtems.org> wrote:
>
> I have a couple minor notes below. More important, does this change
> require updating documentation?
I'd have to look to see if this window size detection is documented, I wanted to fix the problem it caused me.
>
> 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?
I don't know. Same with the "vtime" setting, no way to change that either. Rationale:
- It puts what you may want to change in one place.
- I'm not a fan of "#define".
- I didn't want to add a public interface to tweak things (then I *would* need to document).
- It was useful. I left it in to make it clear how to compile-time turn it on if you need it.
I can make the "dbf" behavior pre-processor enabled at compile-time. Or take it out.
I would leave the "vtime" static, there was no way to change the previous timeout either.
>
>> +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
Sure.
>
>> + 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
Peter
-----------------
Peter Dufault
HD Associates, Inc. Software and System Engineering
More information about the devel
mailing list