From 979622493646be48d53d220baedd504647cf0541 Mon Sep 17 00:00:00 2001 From: Emmanuel Ledoux Date: Tue, 27 May 2014 11:33:10 +0200 Subject: [PATCH] - winpr-comm: got a finalized version of IOCTL_SERIAL_WAIT_ON_MASK - serial: cleaning up the code --- channels/serial/client/serial_main.c | 85 +++++++++++++------- winpr/include/winpr/comm.h | 2 +- winpr/libwinpr/comm/comm.c | 13 ++- winpr/libwinpr/comm/comm.h | 7 +- winpr/libwinpr/comm/comm_ioctl.h | 1 - winpr/libwinpr/comm/comm_serial_sys.c | 110 ++++++++++++++++++++++---- winpr/libwinpr/handle/handle.c | 11 +++ 7 files changed, 175 insertions(+), 54 deletions(-) diff --git a/channels/serial/client/serial_main.c b/channels/serial/client/serial_main.c index b45ff2f2e..4294ab33b 100644 --- a/channels/serial/client/serial_main.c +++ b/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 - * a test/debug purpose. NB: two IRPs could not occur at the - * same time, typically two concurent Read/Write - * operations. */ + /* for a test/debug purpose, uncomment the code below to get a + * single thread for all IRPs. NB: two IRPs could not be + * processed at the same time, typically two concurent + * 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->IrpThreadToTerminateCount > 0) + if (serial->IrpThreadToBeTerminatedCount > 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: - * behavior documented somewhere?). + * at least observed with IOCTL_SERIAL_WAIT_ON_MASK and + * 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); - - /* FIXME: asserts that the previous thread's IRP is - * well the same request by checking more - * details. Need an access to the IRP object used by - * previousIrpThread */ + assert(FALSE); /* unimplemented */ + + /* TODO: asserts that previousIrpThread handles well + * the same request by checking more 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); - /* TODO: FIXME: WaitForMultipleObjects() not yet implemented for threads */ + assert(FALSE); /* unimplemented */ + + /* 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() */ - - serial->IrpThreadToTerminateCount = 0; + /* IrpThreads content only modified by create_irp_thread() */ + serial->IrpThreads = ListDictionary_New(FALSE); + serial->IrpThreadToBeTerminatedCount = 0; InitializeCriticalSection(&serial->TerminatingIrpThreadsLock); WLog_Init(); diff --git a/winpr/include/winpr/comm.h b/winpr/include/winpr/comm.h index 290f7367d..c7bc8babc 100644 --- a/winpr/include/winpr/comm.h +++ b/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); diff --git a/winpr/libwinpr/comm/comm.c b/winpr/libwinpr/comm/comm.c index a32f4cd7a..36b2a53e5 100644 --- a/winpr/libwinpr/comm/comm.c +++ b/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 diff --git a/winpr/libwinpr/comm/comm.h b/winpr/libwinpr/comm/comm.h index cbd4b15f0..14e862235 100644 --- a/winpr/libwinpr/comm/comm.h +++ b/winpr/libwinpr/comm/comm.h @@ -24,6 +24,7 @@ #ifndef _WIN32 #include +#include #include @@ -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 */ diff --git a/winpr/libwinpr/comm/comm_ioctl.h b/winpr/libwinpr/comm/comm_ioctl.h index 107fe2cd4..cf3f642c0 100644 --- a/winpr/libwinpr/comm/comm_ioctl.h +++ b/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; diff --git a/winpr/libwinpr/comm/comm_serial_sys.c b/winpr/libwinpr/comm/comm_serial_sys.c index 8fc9ea8d2..ea87e353c 100644 --- a/winpr/libwinpr/comm/comm_serial_sys.c +++ b/winpr/libwinpr/comm/comm_serial_sys.c @@ -26,8 +26,9 @@ #include #include #include -#include #include +#include +#include #include @@ -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(¤tCounters, 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: - // TMP: TODO: wait also on a PendingEvents modification, and a new identical IRP - /* while (TRUE) */ - { - SERIAL_STATUS serialStatus; + /* UGLY: removes the STOP bit set by an initial _set_wait_mask() */ + pComm->PendingEvents &= ~SERIAL_EV_FREERDP_STOP; - /* NB: also ensures PendingEvents to be up to date */ - ZeroMemory(&serialStatus, sizeof(SERIAL_STATUS)); - if (!_get_commstatus(pComm, &serialStatus)) + while (TRUE) + { + 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); - SetLastError(ERROR_IO_PENDING); /* see: WaitCommEvent's help */ + DEBUG_WARN("_wait_on_mask, unexpected return, WaitEventMask=0X%lX", pComm->WaitEventMask); + assert(FALSE); return FALSE; } diff --git a/winpr/libwinpr/handle/handle.c b/winpr/libwinpr/handle/handle.c index 855b7db59..5a40d632a 100644 --- a/winpr/libwinpr/handle/handle.c +++ b/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;