Package: pose
Version: 3.5-7
Severity: normal
Tags: patch
Summary: The previous code incorrectly tested the file descriptor
coming from an open function with <=0 as being an error along with
using 0 as an invalid file descriptor. This caused pose to no longer
be able to open a serial port until it was restarted if a serial port
open failed. I also corrected the PRINTF statements to display the
correct function name.
More detail: When the port is available pose opens it, reads it and
closes it when finished. The previous pose code would check the
return value of open with <=0 and treat it as an error when the
statement is true. When it detected an error it set the file
descriptor to 0 and returned an error code from the open member
function. When the port can't be opened, DestroyCommThreads and
CloseCommPort are both called twice. DestroyCommThreads bails without
doing anything, but CloseCommPort closes file descriptor 0 each time,
even though EmHostTransportSerial object doesn't have any open file
descriptors. The first time it closes file descriptor 0 pose looses
standard input, the second time the close call returns error, but pose
doesn't check. Once that happens OpenCommPort always fails because
the serial port will get the next available file descriptor which is
now zero and the check <=0 assumes it failed, even when the serial
port was successfully opened. From here on out the pose will not be
able to use the serial port until it is restarted.
This is also the case if someone specifies a bad or invalid device
file for the serial port, pose will not work again until restarted.
The fix is relatively trivial, it involves treating -1 as the only
invalid file descriptor and protects against multiple calls to the
CloseCommPort function. As to why close is being called twice I don't
know. I didn't look into that.
I also included a little utility I wrote that I found useful to use
the pseudo terminals instead of two real serial ports and a NULL
modem. It is the last two patches, although I'm not sure where to put
them in the pose source tree.
I modified the PRINTF at the top (the default behavior is the previous
code), which you may or may not want to submit upstream. I needed it
to give me more information such as time to compare with strace.
Index: EmTransportSerialUnix.cpp
===================================================================
RCS file:
/data/debian/pose/cvs_dir/pose-3.5_source/SrcUnix/EmTransportSerialUnix.cpp,v
retrieving revision 1.1
retrieving revision 1.5
diff -u -p -r1.1 -r1.5
--- EmTransportSerialUnix.cpp 21 Jun 2005 01:24:01 -0000 1.1
+++ EmTransportSerialUnix.cpp 4 Jul 2005 16:06:43 -0000 1.5
@@ -25,7 +25,16 @@
#include <termios.h> // struct termios
+#if DEBUG > 0
+#include <sys/time.h>
+#include <time.h>
+#define PRINTF(...) do { struct timeval tv; gettimeofday(&tv, NULL); \
+ fprintf(stderr, "%ld:%ld:%ld.%06ld ", (tv.tv_sec/3600)%60,
(tv.tv_sec/60)%60, \
+ tv.tv_sec%60, tv.tv_usec); \
+ fprintf(stderr, __VA_ARGS__) ; fprintf(stderr, "\n"); } while(0)
+#else
#define PRINTF if (!LogSerial ()) ; else LogAppendMsg
+#endif
/***********************************************************************
@@ -596,9 +605,9 @@ void EmTransportSerial::HostGetSerialBau
EmHostTransportSerial::EmHostTransportSerial (void) :
fReadThread (NULL),
fWriteThread (NULL),
- fCommHandle (0),
- fCommSignalPipeA (0),
- fCommSignalPipeB (0),
+ fCommHandle (-1),
+ fCommSignalPipeA (-1),
+ fCommSignalPipeB (-1),
fTimeToQuit (false),
fDataMutex (),
fDataCondition (&fDataMutex),
@@ -626,9 +635,9 @@ EmHostTransportSerial::~EmHostTransportS
{
EmAssert (fReadThread == NULL);
EmAssert (fWriteThread == NULL);
- EmAssert (fCommHandle == 0);
- EmAssert (fCommSignalPipeA == 0);
- EmAssert (fCommSignalPipeB == 0);
+ EmAssert (fCommHandle == -1);
+ EmAssert (fCommSignalPipeA == -1);
+ EmAssert (fCommSignalPipeB == -1);
}
@@ -648,12 +657,17 @@ ErrCode EmHostTransportSerial::OpenCommP
{
EmTransportSerial::PortName portName = config.fPort;
- PRINTF ("EmTransportSerial::HostOpen: attempting to open port \"%s\"",
+ PRINTF ("EmHostTransportSerial::OpenCommPort: attempting to open port
\"%s\"",
portName.c_str());
+ if (fCommHandle != -1)
+ {
+ PRINTF ("EmHostTransportSerial::OpenCommPort: port already
open");
+ return -1;
+ }
if (!portName.empty ())
{
- PRINTF ("EmTransportSerial::HostOpen: Opening serial port...");
+ PRINTF ("EmHostTransportSerial::OpenCommPort: Opening serial
port...");
// An execllent article on serial programming under UNIX
("Linux Serial Port
// Programming Mini-Howto") says to set the following flags in
the open call.
@@ -662,16 +676,14 @@ ErrCode EmHostTransportSerial::OpenCommP
fCommHandle = open(portName.c_str (), O_RDWR | O_NOCTTY |
O_NDELAY);
- if (fCommHandle <= 0)
+ if (fCommHandle == -1)
{
- fCommHandle = 0;
-
return errno;
}
}
else
{
- PRINTF ("EmTransportSerial::HostOpen: No port selected in the
Properties dialog box...");
+ PRINTF ("EmHostTransportSerial::OpenCommPort: No port selected
in the Properties dialog box...");
return -1; // !!! better error number
}
@@ -694,25 +706,29 @@ ErrCode EmHostTransportSerial::OpenCommP
ErrCode EmHostTransportSerial::CreateCommThreads (const
EmTransportSerial::ConfigSerial& /*config*/)
{
- if (fCommHandle)
+ if (fCommHandle == -1)
{
- PRINTF ("EmTransportSerial::HostOpen: Creating serial port
handler threads...");
+ PRINTF ("EmHostTransportSerial::CreateCommThreads: Serial port
not open...");
+ return -1;
+ }
- // Create the pipe used to communicate with CommRead.
+ PRINTF ("EmHostTransportSerial::CreateCommThreads: Creating serial port
handler threads...");
- int filedes[] = { 0, 0 };
- if (pipe (filedes) == 0)
- {
- fCommSignalPipeA = filedes[0]; // for reading
- fCommSignalPipeB = filedes[1]; // for writing
- }
+ // Create the pipe used to communicate with CommRead.
- // Create the threads and start them up.
-
- fTimeToQuit = false;
- fReadThread = omni_thread::create (CommRead, this);
- fWriteThread = omni_thread::create (CommWrite, this);
+ int filedes[] = { 0, 0 };
+ if(pipe (filedes) == -1 )
+ {
+ return errno;
}
+ fCommSignalPipeA = filedes[0]; // for reading
+ fCommSignalPipeB = filedes[1]; // for writing
+
+ // Create the threads and start them up.
+
+ fTimeToQuit = false;
+ fReadThread = omni_thread::create (CommRead, this);
+ fWriteThread = omni_thread::create (CommWrite, this);
return errNone;
}
@@ -732,9 +748,10 @@ ErrCode EmHostTransportSerial::CreateCom
ErrCode EmHostTransportSerial::DestroyCommThreads (void)
{
+ PRINTF ("EmHostTransportSerial::DestroyCommThreads: Shutdown and
destroy the comm threads.");
// If never created, nothing to destroy.
- if (!fCommSignalPipeA)
+ if (fCommSignalPipeA == -1)
return errNone;
// Signal the threads to quit.
@@ -767,7 +784,7 @@ ErrCode EmHostTransportSerial::DestroyCo
close (fCommSignalPipeA);
close (fCommSignalPipeB);
- fCommSignalPipeA = fCommSignalPipeB = 0;
+ fCommSignalPipeA = fCommSignalPipeB = -1;
return errNone;
}
@@ -787,9 +804,14 @@ ErrCode EmHostTransportSerial::DestroyCo
ErrCode EmHostTransportSerial::CloseCommPort (void)
{
+ if(fCommHandle == -1)
+ {
+ return -1;
+ }
+
(void) close (fCommHandle);
- fCommHandle = 0;
+ fCommHandle = -1;
return errNone;
}
@@ -981,7 +1003,7 @@ void* EmHostTransportSerial::CommRead (v
{
EmHostTransportSerial* This = (EmHostTransportSerial*) data;
- PRINTF ("CommRead starting.");
+ PRINTF ("EmHostTransportSerial::CommRead: starting.");
while (!This->fTimeToQuit)
{
@@ -998,7 +1020,10 @@ void* EmHostTransportSerial::CommRead (v
status = select (maxfd + 1, &read_fds, NULL, NULL, NULL);
if (This->fTimeToQuit)
+ {
+ PRINTF ("EmHostTransportSerial::CommRead: after select
fTimeToQuit is true");
break;
+ }
if (status > 0) // data available
{
@@ -1008,8 +1033,18 @@ void* EmHostTransportSerial::CommRead (v
int len = 1024;
len = read (fd1, buf, len);
+ // Break if read returns error
+ if (len == -1)
+ {
+ PRINTF
("EmHostTransportSerial::CommRead: read error %s",
+ strerror(errno));
+ break;
+ }
if (len == 0)
- break; // port closed
+ {
+ PRINTF
("EmHostTransportSerial::CommRead: read %d bytes", len);
+ break; // port error?
+ }
// Log the data.
if (LogSerialData ())
@@ -1024,7 +1059,7 @@ void* EmHostTransportSerial::CommRead (v
}
}
- PRINTF ("CommRead exitting.");
+ PRINTF ("EmHostTransportSerial::CommRead: exitting.");
return NULL;
}
@@ -1050,7 +1085,7 @@ void* EmHostTransportSerial::CommWrite (
{
EmHostTransportSerial* This = (EmHostTransportSerial*) data;
- PRINTF ("CommWrite starting.");
+ PRINTF ("EmHostTransportSerial::CommWrite: starting.");
omni_mutex_lock lock (This->fDataMutex);
@@ -1093,7 +1128,7 @@ void* EmHostTransportSerial::CommWrite (
Platform::DisposeMemory (buf);
}
- PRINTF ("CommWrite exitting.");
+ PRINTF ("EmHostTransportSerial::CommWrite: exitting.");
return NULL;
}
Index: Makefile
===================================================================
RCS file: Makefile
diff -N Makefile
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ Makefile 22 Jun 2005 02:49:46 -0000 1.1
@@ -0,0 +1,3 @@
+CXXFLAGS=-g -Wall
+
+pty2pty:
Index: pty2pty.cc
===================================================================
RCS file: pty2pty.cc
diff -N pty2pty.cc
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ pty2pty.cc 4 Jul 2005 16:44:50 -0000 1.9
@@ -0,0 +1,229 @@
+/* Allows two programs using the pseuto ttys to open and close their
+ * device at will without giving the other program a notification of
+ * the open or close. Very useful with pose as it tends to open and
+ * close the serial port frequently in the middle of a hotsync
+ * operation. It also removes the ordering which program had to be
+ * run first and be waiting for input.
+ *
+ * Copyright (C) 2005 David Fries <[EMAIL PROTECTED]>
+ *
+ * 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
+ * the Free Software Foundation; using version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * This program allows two programs on the same computer to talk
+ * through a tty local null modem like cable. Things like baud rates
+ * are ignored, along with I'm sure other real hardware serial
+ * interfaces, but the termios calls do not fail which allows at least
+ * some programs to work without a problem using it.
+ *
+ * The Unix Palm Pilot utilities use the serial port to connect to the
+ * Palm Pilot's serial port for synchronization etc. The Palm Pilot
+ * emulator has the ability to use a Unix serial port as if it was the
+ * serial port on the Palm Pilot. The normal case as listed in the
+ * emulator manual is to connect a physical serial null modem between
+ * the two ports and talk over that. It is closer to simulating the
+ * connection to a real Palm handheld, but it seems silly to use up
+ * two serial ports to talk from one program to another on the same
+ * computer. The Unix /dev/pty /dev/tty pairs provide a terminal
+ * interface that allows the normal serial setup calls to be executed
+ * ignoring what doesn't make sense for the real hardware such as baud
+ * rate. The problem is the order of operation. The /dev/pty?? must
+ * be opened first, then the /dev/tty?? and if either of them close
+ * the sequence must be started over. Unfortunately pose frequently
+ * closes the serial port making it hard to complete even a single
+ * hotsync operation.
+ *
+ * That is the reason for this program. It opens two /dev/pty ports
+ * and relays data from one to the other. If a program closes the
+ * /dev/tty port this program gets a read error which it retries after
+ * a delay until the /dev/tty port is opened and data can again flow.
+ * The other program need not be notified. An alternate way to
+ * delaying and retrying the read operation is to close the /dev/pty
+ * and open it again. This has the advantage that read will block
+ * until there is data instead of returning EIO. Unfortunately this
+ * introduces a race condition, which with pose frequently opening and
+ * closing the device comes up rather quickly. The race condition is,
+ * pose closes port, pty2pty gets EIO from read, pose opens port,
+ * pty2pty closes port, pty2pty opens port. The pty must be opened
+ * first, and if the pty is closed while the tty is still open the pty
+ * can't be opened again until the tty is closed. At this point pose
+ * has the tty open and pty2pty gets EIO each time it tries to open
+ * the port. This is a real race condition I'm seeing and polling
+ * read is the only way I see to get around it.
+ *
+ * Future improvement might include some circular buffers. For now it
+ * is keep the program simple stupid, besides the kernel has buffers,
+ * why not use them?
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/select.h>
+#include <termios.h>
+#include <time.h>
+#include <unistd.h>
+
+static const char *version = "$Id: pty2pty.cc,v 1.9 2005/07/04 16:44:50 david
Exp $";
+
+//#define DEBUG 1
+
+int main(int argc, char **argv)
+{
+ char *device_name[2] = { NULL, NULL};
+ int fd[2] = { -1, -1};
+ const int buf_size = 4096;
+ int buf[2][buf_size];
+ int buf_count[2] = {0,0};
+ int buf_loc[2] = {0,0};
+ if(argc != 3)
+ {
+ fprintf(stderr, "Usage: %s /dev/pty?? /dev/pty??\n"
+ "\tBoth pty devices are opened and data is forwarded"
+ "from one to the other\n%s\n", argv[0], version);
+ exit(1);
+ }
+ device_name[0] = argv[1];
+ device_name[1] = argv[2];
+
+ fd_set readfds, writefds;
+ int events, result, i;
+
+ /* Open the ports */
+ for(i=0; i<2; ++i)
+ {
+ fd[i] = open(device_name[i], O_RDWR | O_NONBLOCK);
+ if(fd[i] == -1)
+ {
+ fprintf(stderr, "Error opening port %s:%s\n",
+ device_name[i], strerror(errno));
+ exit(1);
+ }
+ struct termios t;
+ if(tcgetattr(fd[i], &t)==-1)
+ {
+ perror("Error getting terminal settings\n");
+ exit(1);
+ }
+ cfmakeraw(&t);
+ if(tcsetattr(fd[i], TCSANOW, &t)==-1)
+ {
+ perror("Error setting terminal settings\n");
+ exit(1);
+ }
+ }
+
+ bool read_EIO[2] = {false,false};
+ struct timeval timeout, *t_ptr;
+ for(;;)
+ {
+ FD_ZERO(&readfds);
+ FD_ZERO(&writefds);
+ t_ptr = NULL;
+ /* Setup select. */
+ for(i=0; i<2; ++i)
+ {
+
+ /* Setup select call. */
+ /* Simple buffer, read or write, if there is data
+ * write, if there isn't data read, setup select
+ * operations for that direction only.
+ */
+ if(read_EIO[i])
+ {
+ /* Do a timeout when the last pass returned
+ * and EIO error.
+ */
+ t_ptr = &timeout;
+ timeout.tv_sec = 0;
+ /* Time timeout can't be too big or the
+ * pilot-xfer gets confused. 15 milliseconds
+ * was working pretty good.
+ */
+ timeout.tv_usec = 15*1000;
+ }
+ else
+ {
+ if(buf_count[i])
+ FD_SET(fd[(i+1)%2], &writefds);
+ else
+ FD_SET(fd[i], &readfds);
+ }
+ }
+ events = select(1+(fd[0]>fd[1] ? fd[0] : fd[1]), &readfds,
+ &writefds, NULL, t_ptr);
+ if(events == -1)
+ {
+ perror("Error calling select");
+ exit(1);
+ }
+ for(i=0; i<2; ++i)
+ {
+ /* Always clear the read error and try again next
+ * pass.
+ */
+ read_EIO[i] = false;
+ if(FD_ISSET(fd[i], &readfds))
+ {
+ result = read(fd[i], buf[i], buf_size);
+ if(result == -1)
+ {
+ if(errno == EAGAIN)
+ {
+ perror(device_name[i]);
+ break;
+ }
+ #if DEBUG
+ fprintf(stderr, "Error reading %s: "
+ "%s\n", device_name[i],
+ strerror(errno));
+ #endif
+ read_EIO[i] = true;
+ break;
+ }
+ else
+ {
+ buf_count[i] = result;
+ buf_loc[i] = 0;
+ }
+ }
+ if(FD_ISSET(fd[(i+1)%2], &writefds))
+ {
+ result = write(fd[(i+1)%2], buf[i]+buf_loc[i],
+ buf_count[i]);
+ if(result == -1)
+ {
+ #if DEBUG
+ fprintf(stderr, "Error writing %s: "
+ "%s\n", device_name[(i+1)%2],
+ strerror(errno));
+ #endif
+ struct timespec req = {100*1000*1000,0};
+ nanosleep(&req, NULL);
+ break;
+ }
+ else
+ {
+ buf_loc[i] += result;
+ buf_count[i] -= result;
+ }
+ }
+ }
+ }
+
+ return 1;
+}
-- System Information:
Debian Release: 3.1
APT prefers unstable
APT policy: (500, 'unstable')
Architecture: i386 (i686)
Shell: /bin/sh linked to /bin/bash
Kernel: Linux 2.6.12-rc6
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)
Versions of packages pose depends on:
ii libc6 2.3.2.ds1-22 GNU C Library: Shared libraries an
ii libfltk1.1c102 1.1.6-5 Fast Light Toolkit shared librarie
ii libgcc1 1:4.0.0-11 GCC support library
ii libstdc++5 1:3.3.6-5 The GNU Standard C++ Library v3
ii libx11-6 4.3.0.dfsg.1-13 X Window System protocol client li
ii libxext6 4.3.0.dfsg.1-13 X Window System miscellaneous exte
ii xlibmesa-gl [libgl1] 4.3.0.dfsg.1-13 Mesa 3D graphics library [XFree86]
ii xlibmesa-glu [libglu1] 4.3.0.dfsg.1-13 Mesa OpenGL utility library [XFree
ii xlibs 4.3.0.dfsg.1-13 X Keyboard Extension (XKB) configu
-- no debconf information
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
