From 8b9c6fcf4a49df39806a9fe8f3a0b1d8a3955ae3 Mon Sep 17 00:00:00 2001 From: WB2OSZ Date: Fri, 25 Mar 2016 22:20:27 -0400 Subject: [PATCH] Fix two problems with KISS pseudo terminal interface. (1) kissattach: Error setting line discipline: TIOCSETD: Device or resource busy (2) Hang, and other resulting problems, when -p command line option used but there was no application reading from the pseudo terminal. --- kiss.c | 149 +++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 97 insertions(+), 52 deletions(-) diff --git a/kiss.c b/kiss.c index f9933ba..a9e6f60 100644 --- a/kiss.c +++ b/kiss.c @@ -1,7 +1,7 @@ // // This file is part of Dire Wolf, an amateur radio packet TNC. // -// Copyright (C) 2011, 2013, 2014 John Langner, WB2OSZ +// Copyright (C) 2011, 2013, 2014, 2016 John Langner, WB2OSZ // // This program is free software: you can redistribute it and/or modify // it under the terms of the GNU General Public License as published by @@ -24,14 +24,13 @@ * Module: kiss.c * * Purpose: Act as a virtual KISS TNC for use by other packet radio applications. + * On Windows, it is a serial port. On Linux, a pseudo terminal. * * Input: * * Outputs: * - * Description: This provides a pseudo terminal for communication with a client application. - * - * It implements the KISS TNC protocol as described in: + * Description: It implements the KISS TNC protocol as described in: * http://www.ka9q.net/papers/kiss.html * * Briefly, a frame is composed of @@ -355,7 +354,7 @@ static MYFDTYPE kiss_open_pt (void) char *pts; struct termios ts; int e; - //int flags; + #if DEBUG text_color_set(DW_COLOR_DEBUG); @@ -397,39 +396,31 @@ static MYFDTYPE kiss_open_pt (void) } /* - * After running for a while on Linux, the write eventually - * blocks if no one is reading from the other side of - * the pseudo terminal. We get stuck on the kiss data - * write and reception stops. + * We had a problem here since the beginning. + * If no one was reading from the other end of the pseudo + * terminal, the buffer space would eventually fill up, + * the write here would block, and the receive decode + * thread would get stuck. * - * I tried using ioctl(,TIOCOUTQ,) to see how much was in - * the queue but that always returned zero. (Ubuntu) - * - * Let's try using non-blocking writes and see if we get - * the EWOULDBLOCK status instead of hanging. + * March 2016 - A "select" was put before the read to + * solve a different problem. With that in place, we can + * now use non-blocking I/O and detect the buffer full + * condition here. */ -#if 0 // this is worse. all writes fail. errno = 0 bad file descriptor - flags = fcntl(fd, F_GETFL, 0); + // text_color_set(DW_COLOR_DEBUG); + // dw_printf("Debug: Try using non-blocking mode for pseudo terminal.\n"); + + int flags = fcntl(fd, F_GETFL, 0); e = fcntl (fd, F_SETFL, flags | O_NONBLOCK); if (e != 0) { text_color_set(DW_COLOR_ERROR); dw_printf ("Can't set pseudo terminal to nonblocking, fcntl returns %d, errno = %d\n", e, errno); perror ("pt fcntl"); } -#endif -#if 0 // same - flags = 1; - e = ioctl (fd, FIONBIO, &flags); - if (e != 0) { - text_color_set(DW_COLOR_ERROR); - dw_printf ("Can't set pseudo terminal to nonblocking, ioctl returns %d, errno = %d\n", e, errno); - perror ("pt ioctl"); - } -#endif + text_color_set(DW_COLOR_INFO); dw_printf("Virtual KISS TNC is available on %s\n", pt_slave_name); - dw_printf("WARNING - Dire Wolf will hang eventually if nothing is reading from it.\n"); #if 1 @@ -632,13 +623,11 @@ static MYFDTYPE kiss_open_nullmodem (char *devicename) * * flen - Length of raw received frame not including the FCS * or -1 for a text string. - * * * Description: Send message to client. * We really don't care if anyone is listening or not. * I don't even know if we can find out. * - * *--------------------------------------------------------------------*/ @@ -702,14 +691,12 @@ void kiss_send_rec_packet (int chan, unsigned char *fbuf, int flen) /* Pseudo terminal for Cygwin and Linux. */ - err = write (pt_master_fd, kiss_buff, (size_t)kiss_len); if (err == -1 && errno == EWOULDBLOCK) { -#if DEBUG text_color_set (DW_COLOR_INFO); - dw_printf ("KISS SEND - discarding message because write would block.\n"); -#endif + dw_printf ("KISS SEND - Discarding message because no one is listening.\n"); + dw_printf ("This happens when you use the -p option and don't read from the pseudo terminal.\n"); } else if (err != kiss_len) { @@ -772,13 +759,6 @@ void kiss_send_rec_packet (int chan, unsigned char *fbuf, int flen) //nullmodem_fd = MYFDERROR; } -#if DEBUG - /* Could wait with GetOverlappedResult but we never */ - /* have an issues in this direction. */ - //text_color_set(DW_COLOR_DEBUG); - //dw_printf ("KISS SEND completed. wrote %d / %d\n", nwritten, kiss_len); -#endif - #else err = write (nullmodem_fd, kiss_buf, (size_t)kiss_len); if (err != len) @@ -796,20 +776,25 @@ void kiss_send_rec_packet (int chan, unsigned char *fbuf, int flen) + /*------------------------------------------------------------------- * - * Name: kiss_listen_thread + * Name: kiss_get * - * Purpose: Wait for messages from an application. + * Purpose: Read one byte from the KISS client app. * - * Global In: nullmodem_fd or pt_master_fd + * Global In: nullmodem_fd (Windows) or pt_master_fd (Linux) * - * Description: Process messages from the client application. + * Returns: one byte (value 0 - 255) or terminate thread on error. + * + * Description: There is room for improvment here. Reading one byte + * at a time is inefficient. We could read a large block + * into a local buffer and return a byte from that most of the time. + * Is it worth the effort? I don't know. With GHz processors and + * the low data rate here it might not make a noticable difference. * *--------------------------------------------------------------------*/ -/* Return one byte (value 0 - 255) or terminate thread on error. */ - static int kiss_get (/* MYFDTYPE fd*/ void ) { @@ -884,18 +869,66 @@ static int kiss_get (/* MYFDTYPE fd*/ void ) #else /* Linux/Cygwin version */ int n = 0; - fd_set fd_in; + fd_set fd_in, fd_ex; int rc; while ( n == 0 ) { - /* Reading from master fd of the pty before the client has connected leads to trouble with kissattach. */ - /* Use select to check if the slave has sent any data before trying to read from it. */ + +/* + * Since the beginning we've always had a couple annoying problems with + * the pseudo terminal KISS interface. + * When using "kissattach" we would sometimes get the error message: + * + * kissattach: Error setting line discipline: TIOCSETD: Device or resource busy + * Are you sure you have enabled MKISS support in the kernel + * or, if you made it a module, that the module is loaded? + * + * martinhpedersen came up with the interesting idea of putting in a "select" + * before the "read" and explained it like this: + * + * "Reading from master fd of the pty before the client has connected leads + * to trouble with kissattach. Use select to check if the slave has sent + * any data before trying to read from it." + * + * "This fix resolves the issue by not reading from the pty's master fd, until + * kissattach has opened and configured the slave. This is implemented using + * select() to wait for data before reading from the master fd." + * + * The submitted code looked like this: + * + * FD_ZERO(&fd_in); + * rc = select(pt_master_fd + 1, &fd_in, NULL, &fd_in, NULL); + * + * That doesn't look right to me for a couple reasons. + * First, I would expect to use FD_SET for the fd. + * Second, using the same bit mask for two arguments doesn't seem + * like a good idea because select modifies them. + * When I tried running it, we don't get the failure message + * anymore but the select never returns so we can't read data from + * the KISS client app. + * + * I think this is what we want. + * + * Tested on Raspian (ARM) and Ubuntu (x86_64). + * We don't get the error from kissattach anymore. + */ + FD_ZERO(&fd_in); - rc = select(pt_master_fd + 1, &fd_in, NULL, &fd_in, NULL); + FD_SET(pt_master_fd, &fd_in); + + FD_ZERO(&fd_ex); + FD_SET(pt_master_fd, &fd_ex); + + rc = select(pt_master_fd + 1, &fd_in, NULL, &fd_ex, NULL); + +#if 0 + text_color_set(DW_COLOR_DEBUG); + dw_printf ("select returns %d, errno=%d, fd=%d, fd_in=%08x, fd_ex=%08x\n", rc, errno, pt_master_fd, *((int*)(&fd_in)), *((int*)(&fd_in))); +#endif if (rc == 0) { - continue; + continue; // When could we get a 0? } if (rc == MYFDERROR @@ -903,7 +936,7 @@ static int kiss_get (/* MYFDTYPE fd*/ void ) { text_color_set(DW_COLOR_ERROR); - dw_printf ("\nError receiving kiss message from client application. Closing %s.\n\n", pt_slave_name); + dw_printf ("\nError receiving KISS message from client application. Closing %s.\n\n", pt_slave_name); perror (""); close (pt_master_fd); @@ -938,6 +971,18 @@ static int kiss_get (/* MYFDTYPE fd*/ void ) } +/*------------------------------------------------------------------- + * + * Name: kiss_listen_thread + * + * Purpose: Read messages from serial port KISS client application. + * + * Global In: nullmodem_fd (Windows) or pt_master_fd (Linux) + * + * Description: Reads bytes from the KISS client app and + * sends them to kiss_rec_byte for processing. + * + *--------------------------------------------------------------------*/ static THREAD_F kiss_listen_thread (void *arg)