Start adding RAII to content_enumeration.cpp

setup/extensions.json

@@ -162,7 +162,7 @@
     {
         "name": "wpd",
         "only": "windows",
-        "sources": "calibre/devices/mtp/windows/utils.cpp calibre/devices/mtp/windows/device_enumeration.cpp calibre/devices/mtp/windows/content_enumeration.cpp calibre/devices/mtp/windows/device.cpp calibre/devices/mtp/windows/wpd.cpp",
+        "sources": "calibre/devices/mtp/windows/device_enumeration.cpp calibre/devices/mtp/windows/content_enumeration.cpp calibre/devices/mtp/windows/device.cpp calibre/devices/mtp/windows/wpd.cpp",
         "headers": "calibre/utils/cpp_binding.h calibre/devices/mtp/windows/global.h calibre/utils/windows/common.h",
         "libraries": "ole32 oleaut32 portabledeviceguids user32",
         "cflags": "/X"

src/calibre/devices/mtp/windows/content_enumeration.cpp

@@ -12,6 +12,25 @@
 #define ADDPROP(x) hr = properties->Add(x); if (FAILED(hr)) { hresult_set_exc("Failed to add property " #x " to filesystem properties collection", hr); properties->Release(); return NULL; }
 
 namespace wpd {
+static int
+pump_waiting_messages() {
+	UINT firstMsg = 0, lastMsg = 0;
+    MSG msg;
+	int result = 0;
+	// Read all of the messages in this next loop,
+	// removing each message as we read it.
+	while (PeekMessage(&msg, NULL, firstMsg, lastMsg, PM_REMOVE)) {
+		// If it's a quit message, we're out of here.
+		if (msg.message == WM_QUIT) {
+			result = 1;
+			break;
+		}
+		// Otherwise, dispatch the message.
+		DispatchMessage(&msg);
+	} // End of PeekMessage while loop
+
+    return result;
+}
 
 static IPortableDeviceKeyCollection* create_filesystem_properties_collection() { // {{{
     IPortableDeviceKeyCollection *properties = NULL;
@@ -42,49 +61,43 @@ static IPortableDeviceKeyCollection* create_filesystem_properties_collection() {
 } // }}}
 
 // Convert properties from COM to python {{{
-static void set_string_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, IPortableDeviceValues *properties) {
+static void
+set_string_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, CComPtr<IPortableDeviceValues> &properties) {
     HRESULT hr;
-    wchar_t *property = NULL;
+	com_wchar_raii property;
-    PyObject *val;
+    hr = properties->GetStringValue(key, property.unsafe_address());
-
-    hr = properties->GetStringValue(key, &property);
     if (SUCCEEDED(hr)) {
-        val = wchar_to_unicode(property);
+		pyobject_raii val(PyUnicode_FromWideChar(property.ptr(), -1));
-        if (val != NULL) {
+        if (val) if (PyDict_SetItemString(dict, pykey, val.ptr()) != 0) PyErr_Clear();
-            PyDict_SetItemString(dict, pykey, val);
-            Py_DECREF(val);
-        }
-        CoTaskMemFree(property);
-    }
-}
-
-static void set_bool_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, IPortableDeviceValues *properties) {
-    BOOL ok = 0;
-    HRESULT hr;
-
-    hr = properties->GetBoolValue(key, &ok);
-    if (SUCCEEDED(hr))
-        PyDict_SetItemString(dict, pykey, (ok)?Py_True:Py_False);
-}
-
-static void set_size_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, IPortableDeviceValues *properties) {
-    ULONGLONG val = 0;
-    HRESULT hr;
-    PyObject *pval;
-
-    hr = properties->GetUnsignedLargeIntegerValue(key, &val);
-
-    if (SUCCEEDED(hr)) {
-        pval = PyLong_FromUnsignedLongLong(val);
-        if (pval != NULL) {
-            PyDict_SetItemString(dict, pykey, pval);
-            Py_DECREF(pval);
-        }
     }
 }
 
 static void
-set_date_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, IPortableDeviceValues *properties) {
+set_bool_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, CComPtr<IPortableDeviceValues> &properties) {
+    BOOL ok = 0;
+    HRESULT hr;
+
+    hr = properties->GetBoolValue(key, &ok);
+    if (SUCCEEDED(hr)) {
+        if (PyDict_SetItemString(dict, pykey, (ok)?Py_True:Py_False) != 0) PyErr_Clear();
+	}
+}
+
+static void
+set_size_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, CComPtr<IPortableDeviceValues> &properties) {
+    ULONGLONG val = 0;
+    HRESULT hr;
+    hr = properties->GetUnsignedLargeIntegerValue(key, &val);
+    if (SUCCEEDED(hr)) {
+        pyobject_raii pval(PyLong_FromUnsignedLongLong(val));
+        if (pval) {
+            if (PyDict_SetItemString(dict, pykey, pval.ptr()) != 0) PyErr_Clear();
+        } else PyErr_Clear();
+    }
+}
+
+static void
+set_date_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, CComPtr<IPortableDeviceValues> &properties) {
 	PROPVARIANT ts = {0};
     if (SUCCEEDED(properties->GetValue(key, &ts))) {
 		SYSTEMTIME st;
@@ -99,15 +112,17 @@ set_date_property(PyObject *dict, REFPROPERTYKEY key, const char *pykey, IPortab
     }
 }
 
-static void set_content_type_property(PyObject *dict, IPortableDeviceValues *properties) {
+static void
+set_content_type_property(PyObject *dict, CComPtr<IPortableDeviceValues> &properties) {
     GUID guid = GUID_NULL;
     BOOL is_folder = 0;
 
     if (SUCCEEDED(properties->GetGuidValue(WPD_OBJECT_CONTENT_TYPE, &guid)) && IsEqualGUID(guid, WPD_CONTENT_TYPE_FOLDER)) is_folder = 1;
-    PyDict_SetItemString(dict, "is_folder", (is_folder) ? Py_True : Py_False);
+    if (PyDict_SetItemString(dict, "is_folder", (is_folder) ? Py_True : Py_False) != 0) PyErr_Clear();
 }
 
-static void set_properties(PyObject *obj, IPortableDeviceValues *values) {
+static void
+set_properties(PyObject *obj, CComPtr<IPortableDeviceValues> &values) {
     set_content_type_property(obj, values);
 
     set_string_property(obj, WPD_OBJECT_PARENT_ID, "parent_id", values);
@@ -171,39 +186,32 @@ public:
 
     HRESULT __stdcall OnProgress(REFGUID Context, IPortableDeviceValuesCollection* values) {
         DWORD num = 0, i;
-        wchar_t *property = NULL;
-        IPortableDeviceValues *properties = NULL;
-        PyObject *temp, *obj, *r;
         HRESULT hr;
 
         if (SUCCEEDED(values->GetCount(&num))) {
             PyEval_RestoreThread(this->thread_state);
             for (i = 0; i < num; i++) {
+				CComPtr<IPortableDeviceValues> properties;
                 hr = values->GetAt(i, &properties);
                 if (SUCCEEDED(hr)) {
-
+					com_wchar_raii property;
-                    hr = properties->GetStringValue(WPD_OBJECT_ID, &property);
+                    hr = properties->GetStringValue(WPD_OBJECT_ID, property.unsafe_address());
                     if (!SUCCEEDED(hr)) continue;
-                    temp = wchar_to_unicode(property);
+					pyobject_raii temp(PyUnicode_FromWideChar(property.ptr(), -1));
-                    CoTaskMemFree(property); property = NULL;
+					if (!temp) { PyErr_Clear(); continue; }
-                    if (temp == NULL) continue;
+					pyobject_raii obj(PyDict_GetItem(this->items, temp.ptr()));
-                    obj = PyDict_GetItem(this->items, temp);
+                    if (!obj) {
-                    if (obj == NULL) {
+						obj.attach(Py_BuildValue("{s:O}", "id", temp.ptr()));
-                        obj = Py_BuildValue("{s:O}", "id", temp);
+						if (!obj) { PyErr_Clear(); continue; }
-                        if (obj == NULL) continue;
+                        if (PyDict_SetItem(this->items, temp.ptr(), obj.ptr()) != 0) { PyErr_Clear(); continue; }
-                        PyDict_SetItem(this->items, temp, obj);
+                    } else Py_INCREF(obj.ptr());
-                        Py_DECREF(obj); // We want a borrowed reference to obj
+                    set_properties(obj.ptr(), properties);
+					pyobject_raii r(PyObject_CallFunction(callback, "OI", obj.ptr(), this->level));
+					if (!r) PyErr_Clear();
+					else if (r && PyObject_IsTrue(r.ptr())) {
+						PyObject *borrowed = PyDict_GetItemString(obj.ptr(), "id");
+						if (borrowed) if (PyList_Append(this->subfolders, borrowed) != 0) PyErr_Clear();
                     }
-                    Py_DECREF(temp);
-
-                    set_properties(obj, properties);
-                    r = PyObject_CallFunction(callback, "OI", obj, this->level);
-                    if (r != NULL && PyObject_IsTrue(r)) {
-                        PyList_Append(this->subfolders, PyDict_GetItemString(obj, "id"));
-                    }
-                    Py_XDECREF(r);
-
-                    properties->Release(); properties = NULL;
                 }
             } // end for loop
             this->thread_state = PyEval_SaveThread();
@@ -322,23 +330,21 @@ end:
 
 // Single get filesystem {{{
 
-static PyObject* get_object_properties(IPortableDeviceProperties *devprops, IPortableDeviceKeyCollection *properties, const wchar_t *object_id) {
+static PyObject*
-    IPortableDeviceValues *values = NULL;
+get_object_properties(IPortableDeviceProperties *devprops, IPortableDeviceKeyCollection *properties, const wchar_t *object_id) {
+    CComPtr<IPortableDeviceValues> values;
     HRESULT hr;
-    PyObject *ans = NULL, *temp = NULL;
 
     Py_BEGIN_ALLOW_THREADS;
     hr = devprops->GetValues(object_id, properties, &values);
     Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to get properties for object", hr); goto end; }
+    if (FAILED(hr)) { hresult_set_exc("Failed to get properties for object", hr); return NULL; }
 
-    ans = Py_BuildValue("{s:N}", "id", wchar_to_unicode(object_id));
+	pyobject_raii id(PyUnicode_FromWideChar(object_id, -1));
-    if (ans == NULL) goto end;
+	if (!id) return NULL;
+    PyObject *ans = Py_BuildValue("{s:O}", "id", id.ptr());
+    if (ans == NULL) return NULL;
     set_properties(ans, values);
-
-end:
-    Py_XDECREF(temp);
-    if (values != NULL) values->Release();
     return ans;
 }
 
@@ -466,9 +472,9 @@ static bool get_files_and_folders(unsigned int level, IPortableDevice *device, I
     if (!ok) goto end;
 
     for (Py_ssize_t i = 0; i < PyList_GET_SIZE(subfolders); i++) {
-        const wchar_t *child_id = unicode_to_wchar(PyList_GET_ITEM(subfolders, i));
+		wchar_raii child_id(PyList_GET_ITEM(subfolders, i));
-        if (child_id == NULL) { ok = false; break; }
+        if (!child_id) { PyErr_Clear(); ok = false; break; }
-        ok = get_files_and_folders(level+1, device, content, bulk_properties, child_id, callback, ans);
+        ok = get_files_and_folders(level+1, device, content, bulk_properties, child_id.ptr(), callback, ans);
         if (!ok) break;
     }
 end:

src/calibre/devices/mtp/windows/device_enumeration.cpp

@@ -9,42 +9,6 @@
 
 namespace wpd {
 
-IPortableDeviceValues *get_client_information() { // {{{
-    HRESULT hr;
-
-    ENSURE_WPD(NULL);
-    CComPtr<IPortableDeviceValues> client_information;
-
-    Py_BEGIN_ALLOW_THREADS;
-	hr = client_information.CoCreateInstance(CLSID_PortableDeviceValues, NULL, CLSCTX_INPROC_SERVER);
-    Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to create IPortableDeviceValues", hr); return NULL; }
-
-    Py_BEGIN_ALLOW_THREADS;
-    hr = client_information->SetStringValue(WPD_CLIENT_NAME, client_info.name.ptr());
-    Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to set client name", hr); return NULL; }
-    Py_BEGIN_ALLOW_THREADS;
-    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_MAJOR_VERSION, client_info.major_version);
-    Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to set major version", hr); return NULL; }
-    Py_BEGIN_ALLOW_THREADS;
-    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_MINOR_VERSION, client_info.minor_version);
-    Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to set minor version", hr); return NULL; }
-    Py_BEGIN_ALLOW_THREADS;
-    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_REVISION, client_info.revision);
-    Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to set revision", hr); return NULL; }
-    //  Some device drivers need to impersonate the caller in order to function correctly.  Since our application does not
-    //  need to restrict its identity, specify SECURITY_IMPERSONATION so that we work with all devices.
-    Py_BEGIN_ALLOW_THREADS;
-    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_SECURITY_QUALITY_OF_SERVICE, SECURITY_IMPERSONATION);
-    Py_END_ALLOW_THREADS;
-    if (FAILED(hr)) { hresult_set_exc("Failed to set quality of service", hr); return NULL; }
-    return client_information.Detach();
-} // }}}
-
 IPortableDevice*
 open_device(const wchar_t *pnp_id, CComPtr<IPortableDeviceValues> &client_information) { // {{{
     CComPtr<IPortableDevice> device;
@@ -151,7 +115,7 @@ get_storage_info(IPortableDevice *device) { // {{{
                         pyobject_raii so(Py_BuildValue("{s:K, s:K, s:K, s:K, s:O, s:O}",
                                 "capacity", capacity, "capacity_objects", capacity_objects, "free_space", free_space, "free_objects", free_objects, "rw", paccess, "id", soid.ptr()));
                         if (!so) return NULL;
-#define A(which, key) { com_wchar_raii buf; if (SUCCEEDED(values->GetStringValue(which, buf.address()))) { \
+#define A(which, key) { com_wchar_raii buf; if (SUCCEEDED(values->GetStringValue(which, buf.unsafe_address()))) { \
 							pyobject_raii d(PyUnicode_FromWideChar(buf.ptr(), -1)); \
 							if (d) PyDict_SetItemString(so.ptr(), key, d.ptr()); \
 							else PyErr_Clear(); \

src/calibre/devices/mtp/windows/global.h

@@ -27,15 +27,6 @@ extern PyObject *WPDError, *NoWPD, *WPDFileBusy;
 // The global device manager
 extern CComPtr<IPortableDeviceManager> portable_device_manager;
 
-// Application info
-typedef struct {
-    wchar_raii name;
-    unsigned int major_version;
-    unsigned int minor_version;
-    unsigned int revision;
-} ClientInfo;
-extern ClientInfo client_info;
-
 // Device type
 typedef struct {
     PyObject_HEAD
@@ -49,10 +40,6 @@ extern PyTypeObject DeviceType;
 
 #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<IPortableDeviceValues> &client_information);
 extern PyObject* get_device_information(CComPtr<IPortableDevice> &device, CComPtr<IPortableDevicePropertiesBulk> &bulk_properties);

src/calibre/devices/mtp/windows/utils.cpp

@@ -1,50 +0,0 @@
-/*
- * utils.cpp
- * Copyright (C) 2012 Kovid Goyal <kovid at kovidgoyal.net>
- *
- * Distributed under terms of the GPL3 license.
- */
-
-#include "global.h"
-
-using namespace wpd;
-
-wchar_t *wpd::unicode_to_wchar(PyObject *o) {
-    wchar_t *buf;
-    Py_ssize_t len;
-    if (o == NULL) return NULL;
-    if (!PyUnicode_Check(o)) {PyErr_Format(PyExc_TypeError, "The python object must be a unicode object"); return NULL;}
-    len = PyUnicode_GET_SIZE(o);
-    buf = (wchar_t *)calloc(len+2, sizeof(wchar_t));
-    if (buf == NULL) { PyErr_NoMemory(); return NULL; }
-    len = PyUnicode_AsWideChar(o, buf, len);
-    if (len == -1) { free(buf); PyErr_Format(PyExc_TypeError, "Invalid python unicode object."); return NULL; }
-    return buf;
-}
-
-PyObject *wpd::wchar_to_unicode(const wchar_t *o) {
-    PyObject *ans;
-    if (o == NULL) return NULL;
-    ans = PyUnicode_FromWideChar(o, -1);
-    if (ans == NULL) PyErr_NoMemory();
-    return ans;
-}
-
-int wpd::pump_waiting_messages() {
-	UINT firstMsg = 0, lastMsg = 0;
-    MSG msg;
-	int result = 0;
-	// Read all of the messages in this next loop,
-	// removing each message as we read it.
-	while (PeekMessage(&msg, NULL, firstMsg, lastMsg, PM_REMOVE)) {
-		// If it's a quit message, we're out of here.
-		if (msg.message == WM_QUIT) {
-			result = 1;
-			break;
-		}
-		// Otherwise, dispatch the message.
-		DispatchMessage(&msg);
-	} // End of PeekMessage while loop
-
-    return result;
-}

src/calibre/devices/mtp/windows/wpd.cpp

@@ -18,7 +18,51 @@ CComPtr<IPortableDeviceManager> wpd::portable_device_manager = NULL;
 // Flag indicating if COM has been initialized
 static int _com_initialized = 0;
 // Application Info
-wpd::ClientInfo wpd::client_info;
+class ClientInfo {
+    public:
+        wchar_raii name;
+        unsigned int major_version;
+        unsigned int minor_version;
+        unsigned int revision;
+        ClientInfo() : name(), major_version(0), minor_version(0), revision(0) {}
+};
+static ClientInfo client_info;
+
+IPortableDeviceValues* wpd::get_client_information() { // {{{
+    HRESULT hr;
+
+    ENSURE_WPD(NULL);
+    CComPtr<IPortableDeviceValues> client_information;
+
+    Py_BEGIN_ALLOW_THREADS;
+	hr = client_information.CoCreateInstance(CLSID_PortableDeviceValues, NULL, CLSCTX_INPROC_SERVER);
+    Py_END_ALLOW_THREADS;
+    if (FAILED(hr)) { hresult_set_exc("Failed to create IPortableDeviceValues", hr); return NULL; }
+
+    Py_BEGIN_ALLOW_THREADS;
+    hr = client_information->SetStringValue(WPD_CLIENT_NAME, client_info.name.ptr());
+    Py_END_ALLOW_THREADS;
+    if (FAILED(hr)) { hresult_set_exc("Failed to set client name", hr); return NULL; }
+    Py_BEGIN_ALLOW_THREADS;
+    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_MAJOR_VERSION, client_info.major_version);
+    Py_END_ALLOW_THREADS;
+    if (FAILED(hr)) { hresult_set_exc("Failed to set major version", hr); return NULL; }
+    Py_BEGIN_ALLOW_THREADS;
+    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_MINOR_VERSION, client_info.minor_version);
+    Py_END_ALLOW_THREADS;
+    if (FAILED(hr)) { hresult_set_exc("Failed to set minor version", hr); return NULL; }
+    Py_BEGIN_ALLOW_THREADS;
+    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_REVISION, client_info.revision);
+    Py_END_ALLOW_THREADS;
+    if (FAILED(hr)) { hresult_set_exc("Failed to set revision", hr); return NULL; }
+    //  Some device drivers need to impersonate the caller in order to function correctly.  Since our application does not
+    //  need to restrict its identity, specify SECURITY_IMPERSONATION so that we work with all devices.
+    Py_BEGIN_ALLOW_THREADS;
+    hr = client_information->SetUnsignedIntegerValue(WPD_CLIENT_SECURITY_QUALITY_OF_SERVICE, SECURITY_IMPERSONATION);
+    Py_END_ALLOW_THREADS;
+    if (FAILED(hr)) { hresult_set_exc("Failed to set quality of service", hr); return NULL; }
+    return client_information.Detach();
+} // }}}
 
 // Module startup/shutdown {{{
 static PyObject *

src/calibre/utils/cpp_binding.h

@@ -17,10 +17,12 @@
 template<typename T, void free_T(void*), T null=reinterpret_cast<T>(NULL)>
 class generic_raii {
 	private:
-		T handle;
 		generic_raii( const generic_raii & ) noexcept;
 		generic_raii & operator=( const generic_raii & ) noexcept ;
 
+	protected:
+		T handle;
+
 	public:
 		explicit generic_raii(T h = null) noexcept : handle(h) {}
 		~generic_raii() noexcept { release(); }
@@ -39,7 +41,14 @@ class generic_raii {
 		explicit operator bool() const noexcept { return handle != null; }
 };
 
-typedef generic_raii<wchar_t*, PyMem_Free> wchar_raii;
+class wchar_raii : public generic_raii<wchar_t*, PyMem_Free> {
+	public:
+		explicit wchar_raii() noexcept {}
+		explicit wchar_raii(PyObject *unicode_object) noexcept {
+			if (!unicode_object || !PyUnicode_Check(unicode_object)) { PyErr_SetString(PyExc_TypeError, "Not a unicode object"); return; }
+			handle = PyUnicode_AsWideCharString(unicode_object, NULL);
+		}
+};
 static inline void python_object_destructor(void *p) { PyObject *x = reinterpret_cast<PyObject*>(p); Py_XDECREF(x); }
 typedef generic_raii<PyObject*, python_object_destructor> pyobject_raii;