[Spice-devel] [PATCH v2] usbclerk: add temporary driver install per session

[Uri Lublin]

On 10/24/2012 02:00 PM, Arnon Gilboa wrote:
> -add message type USB_CLERK_DRIVER_SESSION_INSTALL, bump version to 3
> -handle multiple pipe connections concurrently with thread for each one
> -keep pipe open, and on pipe close - cleanup the session-specific devices
> -add test for temporary driver install and multiple devices
>
> v2: fixed most of Uri's comments:
> -remove the device from the list upon USB_CLERK_DRIVER_REMOVE
>   e.g. prevent the following scenario:
>   client1 install+uninstall, client2 install, client1 disconnect
> -limit the number of concurrent connected clients to 32
> -according to the request type, set device auto_remove flag
>   for driver uninstall upon disconnect
>
> FIXME: prevent uninstall for a device used by other clients
>
> rhbz#845216
> ---
>   usbclerk.cpp     |  113 ++++++++++++++++++++++++++++++++++++-----------------
>   usbclerk.h       |    3 +-
>   usbclerktest.cpp |   79 +++++++++++++++++++++++++------------
>   3 files changed, 132 insertions(+), 63 deletions(-)
>
> diff --git a/usbclerk.cpp b/usbclerk.cpp
> index 9ce12b8..f03dd7d 100644
> --- a/usbclerk.cpp
> +++ b/usbclerk.cpp
> @@ -4,6 +4,7 @@
>   #include<stdio.h>
>   #include<string.h>
>   #include<tchar.h>
> +#include<list>
>   #include "usbclerk.h"
>   #include "usbredirfilter.h"
>   #include "libwdi.h"
> @@ -18,6 +19,7 @@
>   #define USB_CLERK_LOG_PATH          TEXT("%susbclerk.log")
>   #define USB_CLERK_PIPE_TIMEOUT      10000
>   #define USB_CLERK_PIPE_BUF_SIZE     1024
> +#define USB_CLERK_PIPE_MAX_CLIENTS  32
>   #define USB_DRIVER_PATH             "%S\\wdi_usb_driver"
>   #define USB_DRIVER_INFNAME_LEN      64
>   #define USB_DRIVER_INSTALL_RETRIES  10
> @@ -26,6 +28,14 @@
>   #define MAX_DEVICE_HCID_LEN         1024
>   #define MAX_DEVICE_FILTER_LEN       1024
>
> +typedef struct USBDev {
> +    UINT16 vid;
> +    UINT16 pid;
> +    bool auto_remove;
> +} USBDev;
> +
> +typedef std::list<USBDev>  USBDevs;
> +
>   class USBClerk {
>   public:
>       static USBClerk* get();
> @@ -37,7 +47,7 @@ public:
>   private:
>       USBClerk();
>       bool execute();
> -    bool dispatch_message(CHAR *buffer, DWORD bytes, USBClerkReply *reply);
> +    bool dispatch_message(CHAR *buffer, DWORD bytes, USBClerkReply *reply, USBDevs *devs);
>       bool install_winusb_driver(int vid, int pid);
>       bool remove_winusb_driver(int vid, int pid);
>       bool uninstall_inf(HDEVINFO devs, PSP_DEVINFO_DATA dev_info);
> @@ -51,6 +61,7 @@ private:
>       bool dev_filter_check(int vid, int pid, bool *has_winusb);
>       static DWORD WINAPI control_handler(DWORD control, DWORD event_type,
>                                           LPVOID event_data, LPVOID context);
> +    static DWORD WINAPI pipe_thread(LPVOID param);
>       static VOID WINAPI main(DWORD argc, TCHAR * argv[]);
>
>   private:
> @@ -60,7 +71,6 @@ private:
>       struct usbredirfilter_rule *_filter_rules;
>       int _filter_count;
>       char _wdi_path[MAX_PATH];
> -    HANDLE _pipe;
>       bool _running;
>       VDLog* _log;
>   };
> @@ -276,11 +286,9 @@ bool USBClerk::execute()
>   {
>       SECURITY_ATTRIBUTES sec_attr;
>       SECURITY_DESCRIPTOR* sec_desr;
> -    USBClerkReply reply = {{USB_CLERK_MAGIC, USB_CLERK_VERSION,
> -        USB_CLERK_REPLY, sizeof(USBClerkReply)}};
>       CHAR filter_str[MAX_DEVICE_FILTER_LEN];
> -    CHAR buffer[USB_CLERK_PIPE_BUF_SIZE];
> -    DWORD bytes;
> +    HANDLE pipe, thread;
> +    DWORD tid;
>       HKEY hkey;
>       LONG ret;
>
> @@ -296,14 +304,6 @@ bool USBClerk::execute()
>       sec_attr.nLength = sizeof(sec_attr);
>       sec_attr.bInheritHandle = TRUE;
>       sec_attr.lpSecurityDescriptor = sec_desr;
> -    _pipe = CreateNamedPipe(USB_CLERK_PIPE_NAME, PIPE_ACCESS_DUPLEX |
> -                            FILE_FLAG_FIRST_PIPE_INSTANCE,
> -                            PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT, 1,
> -                            USB_CLERK_PIPE_BUF_SIZE, USB_CLERK_PIPE_BUF_SIZE, 0,&sec_attr);
> -    if (_pipe == INVALID_HANDLE_VALUE) {
> -        vd_printf("CreatePipe() failed: %u", GetLastError());
> -        return false;
> -    }
>
>       /* Read filter rules from registry */
>       ret = RegOpenKeyEx(HKEY_LOCAL_MACHINE, L"Software\\USBClerk", 0, KEY_READ,&hkey);
> @@ -323,34 +323,62 @@ bool USBClerk::execute()
>           RegCloseKey(hkey);
>       }
>       while (_running) {
> -        if (!ConnectNamedPipe(_pipe, NULL)&&  GetLastError() != ERROR_PIPE_CONNECTED) {
> -            vd_printf("ConnectNamedPipe() failed: %u", GetLastError());
> +        pipe = CreateNamedPipe(USB_CLERK_PIPE_NAME, PIPE_ACCESS_DUPLEX,
> +                               PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
> +                               USB_CLERK_PIPE_MAX_CLIENTS, USB_CLERK_PIPE_BUF_SIZE,
> +                               USB_CLERK_PIPE_BUF_SIZE, 0,&sec_attr);
> +        if (pipe == INVALID_HANDLE_VALUE) {
> +            vd_printf("CreatePipe() failed: %u", GetLastError());
>               break;
>           }
> -        if (!ReadFile(_pipe,&buffer, sizeof(buffer),&bytes, NULL)) {
> -            vd_printf("ReadFile() failed: %d", GetLastError());
> -            goto disconnect;
> -        }
> -        if (!dispatch_message(buffer, bytes,&reply)) {
> -            goto disconnect;
> +        if (!ConnectNamedPipe(pipe, NULL)&&  GetLastError() != ERROR_PIPE_CONNECTED) {
> +            vd_printf("ConnectNamedPipe() failed: %u", GetLastError());
> +            CloseHandle(pipe);
> +            break;
>           }
> -        if (!WriteFile(_pipe,&reply, sizeof(reply),&bytes, NULL)) {
> -            vd_printf("WriteFile() failed: %d", GetLastError());
> -            goto disconnect;
> +        thread = CreateThread(NULL, 0, pipe_thread, (LPVOID)pipe, 0,&tid);
> +        if (thread == NULL) {
> +            vd_printf("CreateThread() failed: %u", GetLastError());
> +            break;
>           }
> -        FlushFileBuffers(_pipe);
> -disconnect:
> -        DisconnectNamedPipe(_pipe);
> +        CloseHandle(thread);
>       }
>       free(_filter_rules);
> -    CloseHandle(_pipe);
>       return true;
>   }
>   

Ack.

Would be nice in the future to not break (and exit) upon errors but
instead do cleanup, possibly sleep for a while, and continue.