- winpr-comm: got a finalized version of IOCTL_SERIAL_WAIT_ON_MASK

- serial: cleaning up the code

channels/serial/client/serial_main.c

@@ -64,14 +64,16 @@ struct _SERIAL_DEVICE
 	DEVICE device;
 	HANDLE* hComm;
 
-	// TMP: use of log
+	/* TODO: use of log (prefered the old fashion DEBUG_SVC and
+	 * DEBUG_WARN macros for backward compatibility resaons)
+	 */
 	wLog* log;
 	HANDLE MainThread;
 	wMessageQueue* MainIrpQueue;
 
 	/* one thread per pending IRP and indexed according their CompletionId */
 	wListDictionary *IrpThreads;
-	UINT32 IrpThreadToTerminateCount;
+	UINT32 IrpThreadToBeTerminatedCount;
 	CRITICAL_SECTION TerminatingIrpThreadsLock;
 };
 
@@ -509,7 +511,7 @@ static void* irp_thread_func(void* arg)
 	serial_process_irp(data->serial, data->irp);
 
 	EnterCriticalSection(&data->serial->TerminatingIrpThreadsLock);
-	data->serial->IrpThreadToTerminateCount++;
+	data->serial->IrpThreadToBeTerminatedCount++;
 
 	data->irp->Complete(data->irp);
 
@@ -532,17 +534,21 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 	HANDLE irpThread = INVALID_HANDLE_VALUE;
 	HANDLE previousIrpThread;
 
-	/* uncomment the code below to get a single thread per IRP for
+	/* for a test/debug purpose, uncomment the code below to get a
-	 * a test/debug purpose. NB: two IRPs could not occur at the
+	 * single thread for all IRPs. NB: two IRPs could not be
-	 * same time, typically two concurent Read/Write
+	 * processed at the same time, typically two concurent
-	 * operations. */
+	 * Read/Write operations could block each other. */
 	/* serial_process_irp(serial, irp); */
 	/* irp->Complete(irp); */
 	/* return; */
 
 
+	/* NOTE: for good or bad, this implementation relies on the
+	 * server to avoid a flooding of requests. see also _purge().
+	 */
+
 	EnterCriticalSection(&serial->TerminatingIrpThreadsLock);
-	while (serial->IrpThreadToTerminateCount > 0)
+	while (serial->IrpThreadToBeTerminatedCount > 0)
 	{
 		/* Cleaning up termitating and pending irp
 		 * threads. See also: irp_thread_func() */
@@ -572,7 +578,7 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 				CloseHandle(irpThread);
 				ListDictionary_Remove(serial->IrpThreads, (void*)id);
 
-				serial->IrpThreadToTerminateCount--;
+				serial->IrpThreadToBeTerminatedCount--;
 			}
 			else if (waitResult != WAIT_TIMEOUT)
  			{
@@ -585,11 +591,9 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 		}
 		
 
-		assert(serial->IrpThreadToTerminateCount == 0); /* TMP: */
+		if (serial->IrpThreadToBeTerminatedCount > 0)
-
-		if (serial->IrpThreadToTerminateCount > 0)
 		{
-			DEBUG_SVC("%d IRP thread(s) not yet terminated", serial->IrpThreadToTerminateCount);
+			DEBUG_SVC("%d IRP thread(s) not yet terminated", serial->IrpThreadToBeTerminatedCount);
 			Sleep(1); /* 1 ms */
 		}
 	}
@@ -599,8 +603,11 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 	 * sure that the incoming IRP uses well a recycled
 	 * CompletionId or the server sent again an IRP already posted
 	 * which didn't get yet a response (this later server behavior
-	 * at least observed with IOCTL_SERIAL_WAIT_ON_MASK FIXME:
+	 * at least observed with IOCTL_SERIAL_WAIT_ON_MASK and
-	 * behavior documented somewhere?).
+	 * mstsc.exe.
+	 *
+	 * FIXME: behavior documented somewhere? behavior not yet
+	 * observed with FreeRDP).
 	 */
 
 	previousIrpThread = ListDictionary_GetItemValue(serial->IrpThreads, (void*)irp->CompletionId);
@@ -610,13 +617,24 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 
 		DEBUG_SVC("IRP recall: IRP with the CompletionId=%d not yet completed!", irp->CompletionId);
 
-		/* TMP: TODO: taking over the pending IRP or sending a kind of wake up signal to accelerate the pending request */
+		assert(FALSE); /* unimplemented */
-		assert(FALSE);
+
-			
+		/* TODO: asserts that previousIrpThread handles well
-		/* FIXME: asserts that the previous thread's IRP is
+		 * the same request by checking more details. Need an
-		 * well the same request by checking more
+		 * access to the IRP object used by previousIrpThread
-		 * details. Need an access to the IRP object used by
+		 */
-		 * previousIrpThread */
+
+		/* TODO: taking over the pending IRP or sending a kind
+		 * of wake up signal to accelerate the pending
+		 * request
+		 *
+		 * To be considered:
+		 *   if (IoControlCode == IOCTL_SERIAL_WAIT_ON_MASK) {
+		 *       pComm->PendingEvents |= SERIAL_EV_FREERDP_*;
+		 *       sem_post(&comm->PendingEventsSem);
+		 *   }
+		 */
+
 		irp->Discard(irp);
 		return;
 	}
@@ -624,10 +642,15 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 
 	if (ListDictionary_Count(serial->IrpThreads) >= MAX_IRP_THREADS)
 	{
-		DEBUG_WARN("Maximal number of IRP threads reached: %d", ListDictionary_Count(serial->IrpThreads));
+		DEBUG_WARN("Number of IRP threads threshold reached: %d, keep on anyway", ListDictionary_Count(serial->IrpThreads));
 		
-		assert(FALSE);
+		assert(FALSE); /* unimplemented */
-		/* TODO: FIXME: WaitForMultipleObjects() not yet implemented for threads */
+
+		/* TODO: MAX_IRP_THREADS has been thought to avoid a
+		 * flooding of pending requests. Use
+		 * WaitForMultipleObjects() when available in winpr
+		 * for threads.
+		 */
 	}
 
 
@@ -643,6 +666,8 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 	data->serial = serial;
 	data->irp = irp;
 
+	/* data freed by irp_thread_func */
+
 	irpThread = CreateThread(NULL, 
 				 0, 
 				 (LPTHREAD_START_ROUTINE)irp_thread_func, 
@@ -660,7 +685,7 @@ static void create_irp_thread(SERIAL_DEVICE *serial, IRP *irp)
 
 	ListDictionary_Add(serial->IrpThreads, (void*)irp->CompletionId, irpThread);
 
-	return; /* data freed by irp_thread_func */
+	return;
 
   error_handle:
 
@@ -757,7 +782,7 @@ int DeviceServiceEntry(PDEVICE_SERVICE_ENTRY_POINTS pEntryPoints)
 
 	if (!name || (name[0] == '*'))
 	{
-		/* TODO: implement auto detection of parallel ports */
+		/* TODO: implement auto detection of serial ports */
 		return 0;
 	}
 
@@ -788,9 +813,9 @@ int DeviceServiceEntry(PDEVICE_SERVICE_ENTRY_POINTS pEntryPoints)
 
 		serial->MainIrpQueue = MessageQueue_New(NULL);
 
-		serial->IrpThreads = ListDictionary_New(FALSE); /* only handled in create_irp_thread() */
+		/* IrpThreads content only modified by create_irp_thread() */
-
+		serial->IrpThreads = ListDictionary_New(FALSE); 
-		serial->IrpThreadToTerminateCount = 0;
+		serial->IrpThreadToBeTerminatedCount = 0;
 		InitializeCriticalSection(&serial->TerminatingIrpThreadsLock);
 
 		WLog_Init();

winpr/include/winpr/comm.h

@@ -388,7 +388,7 @@ WINPR_API BOOL WaitCommEvent(HANDLE hFile, PDWORD lpEvtMask, LPOVERLAPPED lpOver
  *
  * Did something close to QueryDosDevice() and DefineDosDevice() but with
  * folowing constraints:
- *   - mappings are stored in a static wHashTable (thread safe)
+ *   - mappings are stored in a static array.
  *   - QueryCommDevice returns only the mappings that have been defined through DefineCommDevice()
  */
 WINPR_API BOOL DefineCommDevice(/* DWORD dwFlags,*/ LPCTSTR lpDeviceName, LPCTSTR lpTargetPath);

winpr/libwinpr/comm/comm.c

@@ -750,7 +750,7 @@ typedef struct _COMM_DEVICE
 	LPTSTR path;
 } COMM_DEVICE;
 
-/* FIXME: get a clever data structure */
+/* FIXME: get a clever data structure, see also io.h functions */
 static COMM_DEVICE **_CommDevices = NULL;
 
 #define COMM_DEVICE_MAX	128
@@ -809,7 +809,7 @@ static BOOL _IsReservedCommDeviceName(LPCTSTR lpName)
 			return TRUE;
 	}
 
-	/* TMP: TODO: PRN ? */
+	/* FIXME: what about PRN ? */
 
 	return FALSE;
 }
@@ -1147,9 +1147,14 @@ HANDLE CommCreateFileA(LPCSTR lpDeviceName, DWORD dwDesiredAccess, DWORD dwShare
 	}
 
 
+	if (sem_init(&pComm->PendingEventsSem, 0, 0) < 0)
+	{
+		DEBUG_WARN("sem_init failed, errno=[%d] %s", errno, strerror(errno));
+		SetLastError(ERROR_IO_DEVICE);
+		goto error_handle;
+	}
 
-
+	InitializeCriticalSection(&pComm->PendingEventsLock);
-
 
 	/* The binary/raw mode is required for the redirection but
 	 * only flags that are not handle somewhere-else, except

winpr/libwinpr/comm/comm.h

@@ -24,6 +24,7 @@
 #ifndef _WIN32
 
 #include <linux/serial.h>
+#include <semaphore.h>
 
 #include <winpr/comm.h>
 
@@ -66,9 +67,10 @@ struct winpr_comm
 	 * modify counters */
 	struct serial_icounter_struct counters;
 
-	/* TMP: TODO: sync */
 	ULONG WaitEventMask;
 	ULONG PendingEvents;
+	sem_t PendingEventsSem;
+	CRITICAL_SECTION PendingEventsLock;
 
 	/* NB: CloseHandle() has to free resources */
 };
@@ -77,6 +79,9 @@ typedef struct winpr_comm WINPR_COMM;
 
 void _comm_setRemoteSerialDriver(HANDLE hComm, REMOTE_SERIAL_DRIVER_ID);
 
+/* TMP: TODO: move all specific defines and types here? at least SERIAL_EV_* */
+#define SERIAL_EV_FREERDP_STOP     0x8000 /* bit unused by SERIAL_EV_* */
+
 
 #endif /* _WIN32 */
 

winpr/libwinpr/comm/comm_ioctl.h

@@ -162,7 +162,6 @@ typedef struct _SERIAL_TIMEOUTS
 #define SERIAL_EV_EVENT1           0x0800  
 #define SERIAL_EV_EVENT2           0x1000  
 
-
 typedef struct _SERIAL_QUEUE_SIZE
 {
 	ULONG InSize;

winpr/libwinpr/comm/comm_serial_sys.c

@@ -26,8 +26,9 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/ioctl.h>
-#include <unistd.h>
 #include <termios.h>
+#include <time.h>
+#include <unistd.h>
 
 #include <freerdp/utils/debug.h>
 
@@ -1028,11 +1029,16 @@ static BOOL _set_wait_mask(WINPR_COMM *pComm, const ULONG *pWaitMask)
 
 		pComm->PendingEvents = 0;
 	}
+
+	/* Stops pending IOCTL_SERIAL_WAIT_ON_MASK
+	 * http://msdn.microsoft.com/en-us/library/ff546805%28v=vs.85%29.aspx
+	 */
+	EnterCriticalSection(&pComm->PendingEventsLock);
+	pComm->PendingEvents |= SERIAL_EV_FREERDP_STOP;
+	sem_post(&pComm->PendingEventsSem);
+	LeaveCriticalSection(&pComm->PendingEventsLock);
+
 	
-	// TMP: TODO:
-	// pending wait_on_mask must be stopped with STATUS_SUCCESS
-	// http://msdn.microsoft.com/en-us/library/ff546805%28v=vs.85%29.aspx
-	// and pOutputMask = 0;
 
 	possibleMask = *pWaitMask & _SERIAL_SYS_SUPPORTED_EV_MASK;
 
@@ -1104,6 +1110,7 @@ static BOOL _purge(WINPR_COMM *pComm, const ULONG *pPurgeMask)
 
 
 		// TMP: TODO: intercept this call before CommDeviceIoControl() ?
+		// getting a fd_write, fd_read and fs_iotcl?
 	}
 
 	if (*pPurgeMask & SERIAL_PURGE_RXABORT)
@@ -1154,6 +1161,8 @@ static BOOL _get_commstatus(WINPR_COMM *pComm, SERIAL_STATUS *pCommstatus)
 
 	struct serial_icounter_struct currentCounters; 
 
+	EnterCriticalSection(&pComm->PendingEventsLock);
+
 	ZeroMemory(pCommstatus, sizeof(SERIAL_STATUS));
 
 	ZeroMemory(&currentCounters, sizeof(struct serial_icounter_struct));
@@ -1239,7 +1248,7 @@ static BOOL _get_commstatus(WINPR_COMM *pComm, SERIAL_STATUS *pCommstatus)
 	}
 	else
 	{
-		/* FIXME: "now empty" is ambiguous, need to track previous completed transmission? */
+		/* FIXME: "now empty" from the specs is ambiguous, need to track previous completed transmission? */
 		pComm->PendingEvents &= ~SERIAL_EV_TXEMPTY;
 	}
 
@@ -1269,17 +1278,33 @@ static BOOL _get_commstatus(WINPR_COMM *pComm, SERIAL_STATUS *pCommstatus)
 	}
 	else
 	{
-		/* FIXME: "is 80 percent full" is ambiguous, need to track when it previously occured? */
+		/* FIXME: "is 80 percent full" from the specs is ambiguous, need to track when it previously occured? */
 		pComm->PendingEvents &= ~SERIAL_EV_RX80FULL;
 	}
 
 
 	pComm->counters = currentCounters;
 
+	LeaveCriticalSection(&pComm->PendingEventsLock);
 
 	return TRUE;
 }
 
+static BOOL _refresh_PendingEvents(WINPR_COMM *pComm)
+{
+	SERIAL_STATUS serialStatus;
+
+	/* NB: also ensures PendingEvents to be up to date */
+	ZeroMemory(&serialStatus, sizeof(SERIAL_STATUS));
+	if (!_get_commstatus(pComm, &serialStatus))
+	{
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+
 static void _consume_event(WINPR_COMM *pComm, ULONG *pOutputMask, ULONG event)
 {
 	if ((pComm->WaitEventMask & event) && (pComm->PendingEvents & event))
@@ -1289,25 +1314,31 @@ static void _consume_event(WINPR_COMM *pComm, ULONG *pOutputMask, ULONG event)
 	}
 }
 
+/*
+ * NB: see also: _set_wait_mask()
+ */
 static BOOL _wait_on_mask(WINPR_COMM *pComm, ULONG *pOutputMask)
 {
+	
 	assert(*pOutputMask == 0);
 
-	// TMP: TODO:
+	/* UGLY: removes the STOP bit set by an initial _set_wait_mask() */
-	// TMP: TODO: wait also on a PendingEvents modification, and a new identical IRP
+	pComm->PendingEvents &= ~SERIAL_EV_FREERDP_STOP;
-	/* while (TRUE) */
-	{
-		SERIAL_STATUS serialStatus;
 
-		/* NB: also ensures PendingEvents to be up to date */
+	while (TRUE)
-		ZeroMemory(&serialStatus, sizeof(SERIAL_STATUS));
+	{
-		if (!_get_commstatus(pComm, &serialStatus))
+		struct timespec ts;
+
+		if (!_refresh_PendingEvents(pComm))
 		{
 			return FALSE;
 		}
 
+
 		/* events */
 
+		EnterCriticalSection(&pComm->PendingEventsLock);
+
 		_consume_event(pComm, pOutputMask, SERIAL_EV_RXCHAR);
 		_consume_event(pComm, pOutputMask, SERIAL_EV_RXFLAG);
 		_consume_event(pComm, pOutputMask, SERIAL_EV_TXEMPTY);
@@ -1319,15 +1350,60 @@ static BOOL _wait_on_mask(WINPR_COMM *pComm, ULONG *pOutputMask)
 		_consume_event(pComm, pOutputMask, SERIAL_EV_RING );
 		_consume_event(pComm, pOutputMask, SERIAL_EV_RX80FULL);
 
+		LeaveCriticalSection(&pComm->PendingEventsLock);
+
+		/* NOTE: PendingEvents can be modified from now on but
+		 * not pOutputMask */
+
 		if (*pOutputMask != 0)
 		{
 			/* at least an event occurred */
 			return TRUE;
 		}
+
+
+		/* wait for 1 ms or a modification of PendingEvents */
+
+		if (clock_gettime(CLOCK_REALTIME, &ts) < 0)
+		{
+			DEBUG_WARN("clock_realtime failed, errno=[%d] %s", errno, strerror(errno));
+			SetLastError(ERROR_IO_DEVICE);
+			return FALSE;
+		}
+
+		ts.tv_nsec += 100000000; /* 100 ms */
+		if (ts.tv_nsec > 999999999)
+		{
+			ts.tv_sec++;              /* += 1s */
+			ts.tv_nsec -= 1000000000; /* -= 1s */
+		}
+		if (sem_timedwait(&pComm->PendingEventsSem, &ts) < 0)
+		{
+			assert(errno == ETIMEDOUT);
+
+			if (errno != ETIMEDOUT)
+			{
+				DEBUG_WARN("sem_timedwait failed, errno=[%d] %s", errno, strerror(errno));
+				SetLastError(ERROR_IO_DEVICE);
+				return FALSE;
+			}
+		}
+		
+		if (pComm->PendingEvents & SERIAL_EV_FREERDP_STOP)
+		{
+			EnterCriticalSection(&pComm->PendingEventsLock);
+			pComm->PendingEvents &= ~SERIAL_EV_FREERDP_STOP;
+			LeaveCriticalSection(&pComm->PendingEventsLock);
+
+			/* pOutputMask must remain empty
+			 * http://msdn.microsoft.com/en-us/library/ff546805%28v=vs.85%29.aspx
+			 */
+			return TRUE;
+		}
 	}
 
-	DEBUG_WARN("_wait_on_mask pending on events:0X%lX", pComm->WaitEventMask);
+	DEBUG_WARN("_wait_on_mask, unexpected return, WaitEventMask=0X%lX", pComm->WaitEventMask);
-	SetLastError(ERROR_IO_PENDING); /* see: WaitCommEvent's help */
+	assert(FALSE);
 	return FALSE;
 }
 

winpr/libwinpr/handle/handle.c

@@ -206,6 +206,17 @@ BOOL CloseHandle(HANDLE hObject)
 		if (comm->fd > 0)
 			close(comm->fd);
 
+		/* NOTE: avoided to add a dependency by using a
+		 * function such as _stop_wait_on_mask() */
+		EnterCriticalSection(&comm->PendingEventsLock);
+		comm->PendingEvents |= SERIAL_EV_FREERDP_STOP;
+		sem_post(&comm->PendingEventsSem);
+		LeaveCriticalSection(&comm->PendingEventsLock);
+
+		sem_destroy(&comm->PendingEventsSem); /* FIXME: might be too early? */
+
+		DeleteCriticalSection(&comm->PendingEventsLock);
+
 		free(comm);
 
 		return TRUE;