From 2452fbc92886d66ab7a96213f89e514d0f634aa2 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Wed, 21 Apr 2021 09:48:22 +0530 Subject: [PATCH] Use RAII for get_device_information Also the function to set an error from an hresult must return NULL --- src/calibre/devices/mtp/windows/device.cpp | 11 +- .../mtp/windows/device_enumeration.cpp | 141 ++++++------------ src/calibre/devices/mtp/windows/global.h | 6 +- src/calibre/devices/mtp/windows/utils.cpp | 4 - src/calibre/devices/mtp/windows/wpd.cpp | 3 +- src/calibre/utils/windows/common.h | 10 +- 6 files changed, 63 insertions(+), 112 deletions(-) diff --git a/src/calibre/devices/mtp/windows/device.cpp b/src/calibre/devices/mtp/windows/device.cpp index 6fd65dc58d..45914d0268 100644 --- a/src/calibre/devices/mtp/windows/device.cpp +++ b/src/calibre/devices/mtp/windows/device.cpp @@ -37,12 +37,8 @@ init(Device *self, PyObject *args, PyObject *kwds) if (client_information) { self->device = open_device(self->pnp_id.ptr(), client_information); if (self->device) { - IPortableDevicePropertiesBulk *bulk_properties = NULL; - self->device_information = get_device_information(self->device, &bulk_properties); - if (self->device_information) { - ret = 0; - self->bulk_properties = bulk_properties; - } + self->device_information = get_device_information(self->device, self->bulk_properties); + if (self->device_information) ret = 0; } } return ret; @@ -54,7 +50,8 @@ init(Device *self, PyObject *args, PyObject *kwds) static PyObject* update_data(Device *self, PyObject *args) { PyObject *di = NULL; - di = get_device_information(self->device, NULL); + CComPtr bulk_properties; + di = get_device_information(self->device, bulk_properties); if (di == NULL) return NULL; Py_XDECREF(self->device_information); self->device_information = di; Py_RETURN_NONE; diff --git a/src/calibre/devices/mtp/windows/device_enumeration.cpp b/src/calibre/devices/mtp/windows/device_enumeration.cpp index 6822af49ca..6c624c9b26 100644 --- a/src/calibre/devices/mtp/windows/device_enumeration.cpp +++ b/src/calibre/devices/mtp/windows/device_enumeration.cpp @@ -189,83 +189,77 @@ get_storage_info(IPortableDevice *device) { // {{{ } // }}} PyObject* -get_device_information(CComPtr &device, IPortableDevicePropertiesBulk **pb) { // {{{ - IPortableDeviceContent *content = NULL; - IPortableDeviceProperties *properties = NULL; - IPortableDevicePropertiesBulk *properties_bulk = NULL; - IPortableDeviceKeyCollection *keys = NULL; - IPortableDeviceValues *values = NULL; - IPortableDeviceCapabilities *capabilities = NULL; - IPortableDevicePropVariantCollection *categories = NULL; +get_device_information(CComPtr &device, CComPtr &pb) { // {{{ + CComPtr content = NULL; + CComPtr properties = NULL; + CComPtr keys = NULL; + CComPtr values = NULL; + CComPtr capabilities = NULL; + CComPtr categories = NULL; HRESULT hr; DWORD num_of_categories, i; - LPWSTR temp; ULONG ti; - PyObject *t, *ans = NULL; + PyObject *ans = NULL; const char *type = NULL; Py_BEGIN_ALLOW_THREADS; - hr = CoCreateInstance(CLSID_PortableDeviceKeyCollection, NULL, - CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&keys)); + hr = keys.CoCreateInstance(CLSID_PortableDeviceKeyCollection, NULL, CLSCTX_INPROC_SERVER); Py_END_ALLOW_THREADS; - if (FAILED(hr)) {hresult_set_exc("Failed to create IPortableDeviceKeyCollection", hr); goto end;} + if (FAILED(hr)) return hresult_set_exc("Failed to create IPortableDeviceKeyCollection", hr); - Py_BEGIN_ALLOW_THREADS; - hr = keys->Add(WPD_DEVICE_PROTOCOL); +#define A(what) hr = keys->Add(what); if (FAILED(hr)) { return hresult_set_exc("Failed to add key " #what " to IPortableDeviceKeyCollection", hr); } + A(WPD_DEVICE_PROTOCOL); // Despite the MSDN documentation, this does not exist in PortableDevice.h // hr = keys->Add(WPD_DEVICE_TRANSPORT); - hr = keys->Add(WPD_DEVICE_FRIENDLY_NAME); - hr = keys->Add(WPD_DEVICE_MANUFACTURER); - hr = keys->Add(WPD_DEVICE_MODEL); - hr = keys->Add(WPD_DEVICE_SERIAL_NUMBER); - hr = keys->Add(WPD_DEVICE_FIRMWARE_VERSION); - hr = keys->Add(WPD_DEVICE_TYPE); - Py_END_ALLOW_THREADS; - if (FAILED(hr)) {hresult_set_exc("Failed to add keys to IPortableDeviceKeyCollection", hr); goto end;} + A(WPD_DEVICE_FRIENDLY_NAME); + A(WPD_DEVICE_MANUFACTURER); + A(WPD_DEVICE_MODEL); + A(WPD_DEVICE_SERIAL_NUMBER); + A(WPD_DEVICE_FIRMWARE_VERSION); + A(WPD_DEVICE_TYPE); +#undef A Py_BEGIN_ALLOW_THREADS; hr = device->Content(&content); Py_END_ALLOW_THREADS; - if (FAILED(hr)) {hresult_set_exc("Failed to get IPortableDeviceContent", hr); goto end; } + if (FAILED(hr)) return hresult_set_exc("Failed to get IPortableDeviceContent", hr); Py_BEGIN_ALLOW_THREADS; hr = content->Properties(&properties); Py_END_ALLOW_THREADS; - if (FAILED(hr)) {hresult_set_exc("Failed to get IPortableDeviceProperties", hr); goto end; } + if (FAILED(hr)) return hresult_set_exc("Failed to get IPortableDeviceProperties", hr); Py_BEGIN_ALLOW_THREADS; hr = properties->GetValues(WPD_DEVICE_OBJECT_ID, keys, &values); Py_END_ALLOW_THREADS; - if(FAILED(hr)) {hresult_set_exc("Failed to get device info", hr); goto end; } + if(FAILED(hr)) return hresult_set_exc("Failed to get device info", hr); Py_BEGIN_ALLOW_THREADS; hr = device->Capabilities(&capabilities); Py_END_ALLOW_THREADS; - if(FAILED(hr)) {hresult_set_exc("Failed to get device capabilities", hr); goto end; } + if(FAILED(hr)) return hresult_set_exc("Failed to get device capabilities", hr); Py_BEGIN_ALLOW_THREADS; hr = capabilities->GetFunctionalCategories(&categories); Py_END_ALLOW_THREADS; - if(FAILED(hr)) {hresult_set_exc("Failed to get device functional categories", hr); goto end; } + if(FAILED(hr)) return hresult_set_exc("Failed to get device functional categories", hr); Py_BEGIN_ALLOW_THREADS; hr = categories->GetCount(&num_of_categories); Py_END_ALLOW_THREADS; - if(FAILED(hr)) {hresult_set_exc("Failed to get device functional categories number", hr); goto end; } + if(FAILED(hr)) return hresult_set_exc("Failed to get device functional categories number", hr); ans = PyDict_New(); - if (ans == NULL) {PyErr_NoMemory(); goto end;} + if (ans == NULL) return NULL; - if (SUCCEEDED(values->GetStringValue(WPD_DEVICE_PROTOCOL, &temp))) { - t = PyUnicode_FromWideChar(temp, wcslen(temp)); - if (t != NULL) {PyDict_SetItemString(ans, "protocol", t); Py_DECREF(t);} - CoTaskMemFree(temp); - } +#define S(what, key) { \ + com_wchar_raii temp; \ + if (SUCCEEDED(values->GetStringValue(what, &temp))) { \ + pyobject_raii t(PyUnicode_FromWideChar(temp.ptr(), -1)); \ + if (t) if (PyDict_SetItemString(ans, key, t.ptr()) != 0) PyErr_Clear(); \ + }} - // if (SUCCEEDED(values->GetUnsignedIntegerValue(WPD_DEVICE_TRANSPORT, &ti))) { - // PyDict_SetItemString(ans, "isusb", (ti == WPD_DEVICE_TRANSPORT_USB) ? Py_True : Py_False); - // t = PyLong_FromUnsignedLong(ti); - // } + S(WPD_DEVICE_PROTOCOL, "protocol"); if (SUCCEEDED(values->GetUnsignedIntegerValue(WPD_DEVICE_TYPE, &ti))) { type = "unknown"; @@ -285,57 +279,29 @@ get_device_information(CComPtr &device, IPortableDeviceProperti case WPD_DEVICE_TYPE_GENERIC: break; } - t = PyUnicode_FromString(type); - if (t != NULL) { - PyDict_SetItemString(ans, "type", t); Py_DECREF(t); - } + pyobject_raii t(PyUnicode_FromString(type)); + if (t) PyDict_SetItemString(ans, "type", t.ptr()); } - if (SUCCEEDED(values->GetStringValue(WPD_DEVICE_FRIENDLY_NAME, &temp))) { - t = PyUnicode_FromWideChar(temp, wcslen(temp)); - if (t != NULL) {PyDict_SetItemString(ans, "friendly_name", t); Py_DECREF(t);} - CoTaskMemFree(temp); - } + S(WPD_DEVICE_FRIENDLY_NAME, "friendly_name"); + S(WPD_DEVICE_MANUFACTURER, "manufacturer_name"); + S(WPD_DEVICE_MODEL, "model_name"); + S(WPD_DEVICE_SERIAL_NUMBER, "serial_number"); + S(WPD_DEVICE_FIRMWARE_VERSION, "device_version"); +#undef S - if (SUCCEEDED(values->GetStringValue(WPD_DEVICE_MANUFACTURER, &temp))) { - t = PyUnicode_FromWideChar(temp, wcslen(temp)); - if (t != NULL) {PyDict_SetItemString(ans, "manufacturer_name", t); Py_DECREF(t);} - CoTaskMemFree(temp); - } - - if (SUCCEEDED(values->GetStringValue(WPD_DEVICE_MODEL, &temp))) { - t = PyUnicode_FromWideChar(temp, wcslen(temp)); - if (t != NULL) {PyDict_SetItemString(ans, "model_name", t); Py_DECREF(t);} - CoTaskMemFree(temp); - } - - if (SUCCEEDED(values->GetStringValue(WPD_DEVICE_SERIAL_NUMBER, &temp))) { - t = PyUnicode_FromWideChar(temp, wcslen(temp)); - if (t != NULL) {PyDict_SetItemString(ans, "serial_number", t); Py_DECREF(t);} - CoTaskMemFree(temp); - } - - if (SUCCEEDED(values->GetStringValue(WPD_DEVICE_FIRMWARE_VERSION, &temp))) { - t = PyUnicode_FromWideChar(temp, wcslen(temp)); - if (t != NULL) {PyDict_SetItemString(ans, "device_version", t); Py_DECREF(t);} - CoTaskMemFree(temp); - } - - t = Py_False; - for (i = 0; i < num_of_categories; i++) { + bool has_storage = false; + for (i = 0; i < num_of_categories && !has_storage; i++) { PROPVARIANT pv; PropVariantInit(&pv); if (SUCCEEDED(categories->GetAt(i, &pv)) && pv.puuid != NULL) { - if (IsEqualGUID(WPD_FUNCTIONAL_CATEGORY_STORAGE, *pv.puuid)) { - t = Py_True; - } + if (IsEqualGUID(WPD_FUNCTIONAL_CATEGORY_STORAGE, *pv.puuid)) has_storage = true; } PropVariantClear(&pv); - if (t == Py_True) break; } - PyDict_SetItemString(ans, "has_storage", t); + PyDict_SetItemString(ans, "has_storage", has_storage ? Py_True : Py_False); - if (t == Py_True) { + if (has_storage) { pyobject_raii storage(get_storage_info(device)); if (!storage) { pyobject_raii exc_type, exc_value, exc_tb; @@ -344,7 +310,8 @@ get_device_information(CComPtr &device, IPortableDeviceProperti PyErr_NormalizeException(&exc_type, &exc_value, &exc_tb); PyDict_SetItemString(ans, "storage_error", exc_value.ptr()); } else { - PyDict_SetItemString(ans, "storage_error", PyUnicode_FromString("get_storage_info() failed without an error set")); + pyobject_raii t(PyUnicode_FromString("get_storage_info() failed without an error set")); + if (t) PyDict_SetItemString(ans, "storage_error", t.ptr()); } } else { PyDict_SetItemString(ans, "storage", storage.ptr()); @@ -352,19 +319,9 @@ get_device_information(CComPtr &device, IPortableDeviceProperti } Py_BEGIN_ALLOW_THREADS; - hr = properties->QueryInterface(IID_PPV_ARGS(&properties_bulk)); + hr = properties->QueryInterface(IID_PPV_ARGS(&pb)); Py_END_ALLOW_THREADS; PyDict_SetItemString(ans, "has_bulk_properties", (FAILED(hr)) ? Py_False: Py_True); - if (pb != NULL) *pb = (SUCCEEDED(hr)) ? properties_bulk : NULL; - -end: - if (keys != NULL) keys->Release(); - if (values != NULL) values->Release(); - if (properties != NULL) properties->Release(); - if (properties_bulk != NULL && pb == NULL) properties_bulk->Release(); - if (content != NULL) content->Release(); - if (capabilities != NULL) capabilities->Release(); - if (categories != NULL) categories->Release(); return ans; } // }}} diff --git a/src/calibre/devices/mtp/windows/global.h b/src/calibre/devices/mtp/windows/global.h index 88d51f5dcb..4e92691713 100644 --- a/src/calibre/devices/mtp/windows/global.h +++ b/src/calibre/devices/mtp/windows/global.h @@ -47,15 +47,15 @@ typedef struct { } Device; extern PyTypeObject DeviceType; -// Utility functions -PyObject *hresult_set_exc(const char *msg, HRESULT hr); +#define hresult_set_exc(msg, hr) set_error_from_hresult(wpd::WPDError, __FILE__, __LINE__, hr, msg) + wchar_t *unicode_to_wchar(PyObject *o); PyObject *wchar_to_unicode(const wchar_t *o); int pump_waiting_messages(); extern IPortableDeviceValues* get_client_information(); extern IPortableDevice* open_device(const wchar_t *pnp_id, CComPtr &client_information); -extern PyObject* get_device_information(CComPtr &device, IPortableDevicePropertiesBulk **bulk_properties); +extern PyObject* get_device_information(CComPtr &device, CComPtr &bulk_properties); extern PyObject* get_filesystem(IPortableDevice *device, const wchar_t *storage_id, IPortableDevicePropertiesBulk *bulk_properties, PyObject *callback); extern PyObject* get_file(IPortableDevice *device, const wchar_t *object_id, PyObject *dest, PyObject *callback); extern PyObject* create_folder(IPortableDevice *device, const wchar_t *parent_id, const wchar_t *name); diff --git a/src/calibre/devices/mtp/windows/utils.cpp b/src/calibre/devices/mtp/windows/utils.cpp index 4d7976e116..93e13f30c0 100644 --- a/src/calibre/devices/mtp/windows/utils.cpp +++ b/src/calibre/devices/mtp/windows/utils.cpp @@ -9,10 +9,6 @@ using namespace wpd; -PyObject *wpd::hresult_set_exc(const char *msg, HRESULT hr) { - return error_from_hresult(hr, msg); -} - wchar_t *wpd::unicode_to_wchar(PyObject *o) { wchar_t *buf; Py_ssize_t len; diff --git a/src/calibre/devices/mtp/windows/wpd.cpp b/src/calibre/devices/mtp/windows/wpd.cpp index b8a074dde7..5741c6f5de 100644 --- a/src/calibre/devices/mtp/windows/wpd.cpp +++ b/src/calibre/devices/mtp/windows/wpd.cpp @@ -140,7 +140,8 @@ wpd_device_info(PyObject *self, PyObject *args) { CComPtr client_information = get_client_information(); if (client_information) { device = open_device(pnp_id.ptr(), client_information); - if (device) ans = get_device_information(device, NULL); + CComPtr properties_bulk; + if (device) ans = get_device_information(device, properties_bulk); } if (device) device->Close(); diff --git a/src/calibre/utils/windows/common.h b/src/calibre/utils/windows/common.h index 872c163633..ff8263d290 100644 --- a/src/calibre/utils/windows/common.h +++ b/src/calibre/utils/windows/common.h @@ -14,17 +14,17 @@ #define arraysz(x) (sizeof(x)/sizeof(x[0])) static inline PyObject* -set_error_from_hresult(const char *file, const int line, const HRESULT hr, const char *prefix="", PyObject *name=NULL) { +set_error_from_hresult(PyObject *exc_type, const char *file, const int line, const HRESULT hr, const char *prefix="", PyObject *name=NULL) { _com_error err(hr); LPCWSTR msg = err.ErrorMessage(); PyObject *pmsg = PyUnicode_FromWideChar(msg, -1); PyObject *ans; - if (name) ans = PyErr_Format(PyExc_OSError, "%s:%d:%s:[%li] %V: %S", file, line, prefix, hr, pmsg, "Out of memory", name); - else ans = PyErr_Format(PyExc_OSError, "%s:%d:%s:[%li] %V", file, line, prefix, hr, pmsg, "Out of memory"); + if (name) ans = PyErr_Format(exc_type, "%s:%d:%s:[%li] %V: %S", file, line, prefix, hr, pmsg, "Out of memory", name); + else ans = PyErr_Format(exc_type, "%s:%d:%s:[%li] %V", file, line, prefix, hr, pmsg, "Out of memory"); Py_CLEAR(pmsg); - return ans; + return NULL; } -#define error_from_hresult(hr, ...) set_error_from_hresult(__FILE__, __LINE__, hr, __VA_ARGS__) +#define error_from_hresult(hr, ...) set_error_from_hresult(PyExc_OSError, __FILE__, __LINE__, hr, __VA_ARGS__) template class generic_raii {