Re: [win-pv-devel] [PATCH v2] Handle QueryRemoveFailed

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Owen Smith
> Sent: 19 October 2018 15:18
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH v2] Handle QueryRemoveFailed
> 
> Its possible for the QueryRemove to fail, in which case, the xenagent
> should re-open the device and inform the client code.
> This allows a the XenIface device to re-write the control/feature-*
> flags if XenIface failed the QueryRemove
> 
> Also replaces Tabs with 4 spaces.
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

Acked-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
>  src/xenagent/devicelist.cpp | 187 +++++++++++++++++++++++++++++----------
> -----
>  src/xenagent/devicelist.h   |  13 +--
>  2 files changed, 132 insertions(+), 68 deletions(-)
> 
> diff --git a/src/xenagent/devicelist.cpp b/src/xenagent/devicelist.cpp
> index e0583f9..4acf532 100644
> --- a/src/xenagent/devicelist.cpp
> +++ b/src/xenagent/devicelist.cpp
> @@ -36,6 +36,8 @@
> 
>  #include "devicelist.h"
> 
> +#define BUFFER_SIZE 127
> +
>  // deal with SetupApi and RegisterDeviceNotification using different
> string types
>  static std::wstring Convert(const char* str)
>  {
> @@ -50,6 +52,19 @@ static std::wstring Convert(const wchar_t* wstr)
>      return std::wstring(wstr);
>  }
> 
> +static void DebugPrint(const wchar_t* fmt, ...)
> +{
> +    wchar_t buffer[BUFFER_SIZE + 1];
> +    va_list args;
> +
> +    va_start(args, fmt);
> +    _vsnwprintf(buffer, BUFFER_SIZE, fmt, args);
> +    va_end(args);
> +
> +    buffer[BUFFER_SIZE] = 0;
> +    OutputDebugStringW(buffer);
> +}
> +
>  CDevice::CDevice(const wchar_t* path) :
>      m_handle(INVALID_HANDLE_VALUE), m_path(path), m_notify(NULL)
>  {
> @@ -58,6 +73,7 @@ CDevice::CDevice(const wchar_t* path) :
>  /*virtual*/ CDevice::~CDevice()
>  {
>      Close();
> +    Unregister();
>  }
> 
>  const wchar_t* CDevice::Path() const
> @@ -65,7 +81,7 @@ const wchar_t* CDevice::Path() const
>      return m_path.c_str();
>  }
> 
> -HANDLE CDevice::Open(HANDLE svc)
> +bool CDevice::Open()
>  {
>      Close();
> 
> @@ -76,8 +92,21 @@ HANDLE CDevice::Open(HANDLE svc)
>                             OPEN_EXISTING,
>                             0,
>                             NULL);
> +
> +    return (m_handle != INVALID_HANDLE_VALUE);
> +}
> +
> +void CDevice::Close()
> +{
>      if (m_handle == INVALID_HANDLE_VALUE)
> -        return INVALID_HANDLE_VALUE;
> +        return;
> +    CloseHandle(m_handle);
> +    m_handle = INVALID_HANDLE_VALUE;
> +}
> +
> +HDEVNOTIFY CDevice::Register(HANDLE svc)
> +{
> +    Unregister();
> 
>      DEV_BROADCAST_HANDLE devhdl = { 0 };
>      devhdl.dbch_size = sizeof(devhdl);
> @@ -85,20 +114,16 @@ HANDLE CDevice::Open(HANDLE svc)
>      devhdl.dbch_handle = m_handle;
> 
>      m_notify = RegisterDeviceNotification(svc, &devhdl,
> DEVICE_NOTIFY_SERVICE_HANDLE);
> -    if (m_notify == NULL) {
> -        Close();
> -        return INVALID_HANDLE_VALUE;
> -    }
> -
> -    return m_handle;
> +    return m_notify;
>  }
> 
> -void CDevice::Close()
> +void CDevice::Unregister()
>  {
> -    if (m_handle == INVALID_HANDLE_VALUE)
> +    if (m_notify == NULL)
>          return;
> -    CloseHandle(m_handle);
> -    m_handle = INVALID_HANDLE_VALUE;
> +
> +    UnregisterDeviceNotification(m_notify);
> +    m_notify = NULL;
>  }
> 
>  bool CDevice::Write(void *buf, DWORD bufsz, DWORD *bytes /* = NULL*/)
> @@ -198,9 +223,9 @@ bool CDeviceList::Start(HANDLE handle, IDeviceCreator*
> impl)
>                                              len,
>                                              NULL,
>                                              NULL)) {
> -            OnDeviceAdded(Convert((const char*)detail->DevicePath));
> +            DeviceArrival(Convert((const char*)detail->DevicePath));
>          }
> -        delete [] detail;
> +        delete[] detail;
>          itf.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
>      }
>      SetupDiDestroyDeviceInfoList(info);
> @@ -216,8 +241,8 @@ void CDeviceList::Stop()
>      m_notify = NULL;
> 
>      for (DeviceMap::iterator it = m_devs.begin();
> -            it != m_devs.end();
> -            ++it) {
> +         it != m_devs.end();
> +         ++it) {
>          if (m_impl)
>              m_impl->OnDeviceRemoved(it->second);
>          delete it->second;
> @@ -234,26 +259,33 @@ void CDeviceList::OnDeviceEvent(DWORD evt, LPVOID
> data)
>      hdr = (PDEV_BROADCAST_HDR)data;
>      switch (evt) {
>      case DBT_DEVICEARRIVAL:
> -        if (hdr->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
> -            itf = (PDEV_BROADCAST_DEVICEINTERFACE)hdr;
> -            if (itf->dbcc_classguid == m_guid)
> -                OnDeviceAdded(Convert((const wchar_t*)itf->dbcc_name));
> -        }
> +        if (hdr->dbch_devicetype != DBT_DEVTYP_DEVICEINTERFACE)
> +            break;
> +        itf = (PDEV_BROADCAST_DEVICEINTERFACE)hdr;
> +        if (itf->dbcc_classguid != m_guid)
> +            break;
> +        DeviceArrival(Convert((const wchar_t*)itf->dbcc_name));
> +        break;
> +
> +    case DBT_DEVICEREMOVEPENDING:
> +        if (hdr->dbch_devicetype != DBT_DEVTYP_HANDLE)
> +            break;
> +        hdl = (PDEV_BROADCAST_HANDLE)hdr;
> +        DeviceRemoved(hdl->dbch_hdevnotify);
>          break;
> 
>      case DBT_DEVICEQUERYREMOVE:
> -        if (hdr->dbch_devicetype == DBT_DEVTYP_HANDLE) {
> -            hdl = (PDEV_BROADCAST_HANDLE)hdr;
> -            OnDeviceQueryRemove(hdl->dbch_handle);
> -        }
> +        if (hdr->dbch_devicetype != DBT_DEVTYP_HANDLE)
> +            break;
> +        hdl = (PDEV_BROADCAST_HANDLE)hdr;
> +        DeviceRemovePending(hdl->dbch_hdevnotify);
>          break;
> 
> -    case DBT_DEVICEREMOVEPENDING:
> -        if (hdr->dbch_devicetype == DBT_DEVTYP_HANDLE) {
> -            hdl = (PDEV_BROADCAST_HANDLE)hdr;
> -            UnregisterDeviceNotification(hdl->dbch_hdevnotify);
> -            OnDeviceRemoved(hdl->dbch_handle);
> -        }
> +    case DBT_DEVICEQUERYREMOVEFAILED:
> +        if (hdr->dbch_devicetype != DBT_DEVTYP_HANDLE)
> +            break;
> +        hdl = (PDEV_BROADCAST_HANDLE)hdr;
> +        DeviceRemoveFailed(hdl->dbch_hdevnotify);
>          break;
> 
>      default:
> @@ -267,18 +299,18 @@ void CDeviceList::OnPowerEvent(DWORD evt, LPVOID
> data)
> 
>      switch (evt) {
>      case PBT_APMRESUMESUSPEND:
> -    for (DeviceMap::iterator it = m_devs.begin();
> -         it != m_devs.end();
> -         ++it)
> -        m_impl->OnDeviceResume(it->second);
> -    break;
> +        for (DeviceMap::iterator it = m_devs.begin();
> +             it != m_devs.end();
> +             ++it)
> +            m_impl->OnDeviceResume(it->second);
> +        break;
> 
>      case PBT_APMSUSPEND:
> -    for (DeviceMap::iterator it = m_devs.begin();
> -         it != m_devs.end();
> -         ++it)
> -        m_impl->OnDeviceSuspend(it->second);
> -    break;
> +        for (DeviceMap::iterator it = m_devs.begin();
> +             it != m_devs.end();
> +             ++it)
> +            m_impl->OnDeviceSuspend(it->second);
> +        break;
> 
>      default:
>          break;
> @@ -293,52 +325,81 @@ CDevice* CDeviceList::GetFirstDevice()
>      return it->second;
>  }
> 
> -void CDeviceList::OnDeviceAdded(const std::wstring& path)
> +void CDeviceList::DeviceArrival(const std::wstring& path)
>  {
> +    DebugPrint(L"DeviceArrival(%ws)\n", path.c_str());
>      CDevice* dev;
> -    if (m_impl == NULL)
> -        dev = new CDevice(path.c_str());
> -    else
> +    if (m_impl)
>          dev = m_impl->Create(path.c_str());
> +    else
> +        dev = new CDevice(path.c_str());
>      if (dev == NULL)
> -        return; // create failed
> +        goto fail1;
> 
> -    HANDLE handle = dev->Open(m_handle);
> -    if (handle == INVALID_HANDLE_VALUE) {
> -        delete dev;
> -        return; // open failed
> -    }
> +    if (!dev->Open())
> +        goto fail2;
> 
> -    DeviceMap::iterator it = m_devs.find(handle);
> -    if (it != m_devs.end()) {
> -        delete dev;
> -        return;
> -    }
> +    HDEVNOTIFY nfy = dev->Register(m_handle);
> +    if (nfy == NULL)
> +        goto fail3;
> +
> +    m_devs[nfy] = dev;
> 
> -    m_devs[handle] = dev;
>      if (m_impl)
>          m_impl->OnDeviceAdded(dev);
> +
> +    return;
> +
> +fail3:
> +    DebugPrint(L"fail3\n");
> +fail2:
> +    DebugPrint(L"fail2\n");
> +    delete dev; // handles close() and unregister()
> +fail1:
> +    DebugPrint(L"fail1\n");
> +    return;
>  }
> 
> -void CDeviceList::OnDeviceQueryRemove(HANDLE handle)
> +void CDeviceList::DeviceRemoved(HDEVNOTIFY nfy)
>  {
> -    DeviceMap::iterator it = m_devs.find(handle);
> +    DeviceMap::iterator it = m_devs.find(nfy);
>      if (it == m_devs.end())
>          return; // spurious event?
> 
>      CDevice* dev = it->second;
> +    DebugPrint(L"DeviceRemoved(%ws)\n", dev->Path());
> +
> +    delete dev; // handles unregister()
> +    m_devs.erase(it);
> +}
> +
> +void CDeviceList::DeviceRemovePending(HDEVNOTIFY nfy)
> +{
> +    DeviceMap::iterator it = m_devs.find(nfy);
> +    if (it == m_devs.end())
> +        return; // spurious event?
> +
> +    CDevice* dev = it->second;
> +    DebugPrint(L"DeviceRemovePending(%ws)\n", dev->Path());
> +
>      if (m_impl)
>          m_impl->OnDeviceRemoved(dev);
> +
>      dev->Close();
>  }
> 
> -void CDeviceList::OnDeviceRemoved(HANDLE handle)
> +void CDeviceList::DeviceRemoveFailed(HDEVNOTIFY nfy)
>  {
> -    DeviceMap::iterator it = m_devs.find(handle);
> +    DeviceMap::iterator it = m_devs.find(nfy);
>      if (it == m_devs.end())
>          return; // spurious event?
> 
>      CDevice* dev = it->second;
> -    delete dev;
> -    m_devs.erase(it);
> +    DebugPrint(L"DeviceRemoveFailed(%ws)\n", dev->Path());
> +
> +    if (!dev->Open())
> +        DeviceRemoved(nfy);
> +
> +    if (m_impl)
> +        m_impl->OnDeviceAdded(dev);
>  }
> diff --git a/src/xenagent/devicelist.h b/src/xenagent/devicelist.h
> index 5535ad5..e96df56 100644
> --- a/src/xenagent/devicelist.h
> +++ b/src/xenagent/devicelist.h
> @@ -45,8 +45,10 @@ public:
> 
>      const wchar_t* Path() const;
> 
> -    HANDLE Open(HANDLE svc);
> +    bool Open();
>      void Close();
> +    HDEVNOTIFY Register(HANDLE svc);
> +    void Unregister();
> 
>  protected:
>      bool Write(void *buf, DWORD bufsz, DWORD *bytes = NULL);
> @@ -81,11 +83,12 @@ public:
>      CDevice* GetFirstDevice();
> 
>  private:
> -    void OnDeviceAdded(const std::wstring& path);
> -    void OnDeviceQueryRemove(HANDLE handle);
> -    void OnDeviceRemoved(HANDLE dev);
> +    void DeviceArrival(const std::wstring& path);
> +    void DeviceRemoved(HDEVNOTIFY nfy);
> +    void DeviceRemovePending(HDEVNOTIFY nfy);
> +    void DeviceRemoveFailed(HDEVNOTIFY nfy);
> 
> -    typedef std::map< HANDLE, CDevice* > DeviceMap;
> +    typedef std::map< HDEVNOTIFY, CDevice* > DeviceMap;
> 
>      GUID        m_guid;
>      DeviceMap   m_devs;
> --
> 2.16.2.windows.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/win-pv-devel