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.
This commit is contained in:
WB2OSZ 2016-03-25 22:20:27 -04:00
parent b35a674204
commit 8b9c6fcf4a
1 changed files with 97 additions and 52 deletions

149
kiss.c
View File

@ -1,7 +1,7 @@
// //
// This file is part of Dire Wolf, an amateur radio packet TNC. // 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 // 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 // it under the terms of the GNU General Public License as published by
@ -24,14 +24,13 @@
* Module: kiss.c * Module: kiss.c
* *
* Purpose: Act as a virtual KISS TNC for use by other packet radio applications. * 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: * Input:
* *
* Outputs: * Outputs:
* *
* Description: This provides a pseudo terminal for communication with a client application. * Description: It implements the KISS TNC protocol as described in:
*
* It implements the KISS TNC protocol as described in:
* http://www.ka9q.net/papers/kiss.html * http://www.ka9q.net/papers/kiss.html
* *
* Briefly, a frame is composed of * Briefly, a frame is composed of
@ -355,7 +354,7 @@ static MYFDTYPE kiss_open_pt (void)
char *pts; char *pts;
struct termios ts; struct termios ts;
int e; int e;
//int flags;
#if DEBUG #if DEBUG
text_color_set(DW_COLOR_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 * We had a problem here since the beginning.
* blocks if no one is reading from the other side of * If no one was reading from the other end of the pseudo
* the pseudo terminal. We get stuck on the kiss data * terminal, the buffer space would eventually fill up,
* write and reception stops. * the write here would block, and the receive decode
* thread would get stuck.
* *
* I tried using ioctl(,TIOCOUTQ,) to see how much was in * March 2016 - A "select" was put before the read to
* the queue but that always returned zero. (Ubuntu) * solve a different problem. With that in place, we can
* * now use non-blocking I/O and detect the buffer full
* Let's try using non-blocking writes and see if we get * condition here.
* the EWOULDBLOCK status instead of hanging.
*/ */
#if 0 // this is worse. all writes fail. errno = 0 bad file descriptor // text_color_set(DW_COLOR_DEBUG);
flags = fcntl(fd, F_GETFL, 0); // 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); e = fcntl (fd, F_SETFL, flags | O_NONBLOCK);
if (e != 0) { if (e != 0) {
text_color_set(DW_COLOR_ERROR); text_color_set(DW_COLOR_ERROR);
dw_printf ("Can't set pseudo terminal to nonblocking, fcntl returns %d, errno = %d\n", e, errno); dw_printf ("Can't set pseudo terminal to nonblocking, fcntl returns %d, errno = %d\n", e, errno);
perror ("pt fcntl"); 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); text_color_set(DW_COLOR_INFO);
dw_printf("Virtual KISS TNC is available on %s\n", pt_slave_name); 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 #if 1
@ -633,12 +624,10 @@ static MYFDTYPE kiss_open_nullmodem (char *devicename)
* flen - Length of raw received frame not including the FCS * flen - Length of raw received frame not including the FCS
* or -1 for a text string. * or -1 for a text string.
* *
*
* Description: Send message to client. * Description: Send message to client.
* We really don't care if anyone is listening or not. * We really don't care if anyone is listening or not.
* I don't even know if we can find out. * 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. */ /* Pseudo terminal for Cygwin and Linux. */
err = write (pt_master_fd, kiss_buff, (size_t)kiss_len); err = write (pt_master_fd, kiss_buff, (size_t)kiss_len);
if (err == -1 && errno == EWOULDBLOCK) { if (err == -1 && errno == EWOULDBLOCK) {
#if DEBUG
text_color_set (DW_COLOR_INFO); text_color_set (DW_COLOR_INFO);
dw_printf ("KISS SEND - discarding message because write would block.\n"); dw_printf ("KISS SEND - Discarding message because no one is listening.\n");
#endif dw_printf ("This happens when you use the -p option and don't read from the pseudo terminal.\n");
} }
else if (err != kiss_len) else if (err != kiss_len)
{ {
@ -772,13 +759,6 @@ void kiss_send_rec_packet (int chan, unsigned char *fbuf, int flen)
//nullmodem_fd = MYFDERROR; //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 #else
err = write (nullmodem_fd, kiss_buf, (size_t)kiss_len); err = write (nullmodem_fd, kiss_buf, (size_t)kiss_len);
if (err != 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 ) static int kiss_get (/* MYFDTYPE fd*/ void )
{ {
@ -884,18 +869,66 @@ static int kiss_get (/* MYFDTYPE fd*/ void )
#else /* Linux/Cygwin version */ #else /* Linux/Cygwin version */
int n = 0; int n = 0;
fd_set fd_in; fd_set fd_in, fd_ex;
int rc; int rc;
while ( n == 0 ) { 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); 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) if (rc == 0)
{ {
continue; continue; // When could we get a 0?
} }
if (rc == MYFDERROR if (rc == MYFDERROR
@ -903,7 +936,7 @@ static int kiss_get (/* MYFDTYPE fd*/ void )
{ {
text_color_set(DW_COLOR_ERROR); 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 (""); perror ("");
close (pt_master_fd); 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) static THREAD_F kiss_listen_thread (void *arg)