Patch Detail
Show a patch.
GET /api/patches/563/?format=api
{ "id": 563, "url": "https://patchwork.libcamera.org/api/patches/563/?format=api", "web_url": "https://patchwork.libcamera.org/patch/563/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190212223702.9582-3-laurent.pinchart@ideasonboard.com>", "date": "2019-02-12T22:37:02", "name": "[libcamera-devel,3/3] libcamera: signal: Disconnect signal automatically on slot deletion", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "9741e2895a6679861f79025e77013d925166be2b", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/563/mbox/", "series": [ { "id": 178, "url": "https://patchwork.libcamera.org/api/series/178/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=178", "date": "2019-02-12T22:37:00", "name": "[libcamera-devel,1/3] libcamera: pipeline_handler: Disconnect MediaDevice::disconnected signal", "version": 1, "mbox": "https://patchwork.libcamera.org/series/178/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/563/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/563/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4E9260B21\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Feb 2019 23:37:09 +0100 (CET)", "from pendragon.bb.dnainternet.fi\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7FDE62D6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Feb 2019 23:37:09 +0100 (CET)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1550011029;\n\tbh=GaxYGmc7QyxQAgV7D1XW9eE8WnOUmBIoKXYu7KBkymo=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=BaNzl5wE2QGbRkoZHqlvNtfNqPYOp3IaFg7k8quElPNaIBbIEU4yyfYiZZfiSGfr5\n\t1VfP4Ptp0ZeNhz3ExEd+wUychbRso9RakdnHngdFGhFttr7NfbL1K5dUiHaUaA5JBY\n\taUfB3Fwnrhqw/fbjmmkB17K014fml+hjsdofROO8=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 13 Feb 2019 00:37:02 +0200", "Message-Id": "<20190212223702.9582-3-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.19.2", "In-Reply-To": "<20190212223702.9582-1-laurent.pinchart@ideasonboard.com>", "References": "<20190212223702.9582-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 3/3] libcamera: signal: Disconnect signal\n\tautomatically on slot deletion", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Tue, 12 Feb 2019 22:37:10 -0000" }, "content": "When a signal is connected to a member function slot, the slot is not\ndisconnected when the slot object is deleted. This can lead to calling a\nmember function of a deleted object if the signal isn't disconnected\nmanually by the slot object's destructor.\n\nMake signal handling easier by implementing a base Object class that\ntracks all connected signals and disconnects from them automatically\nwhen the object is deleted, using template specialization resolution in\nthe Signal class.\n\nAs inheriting from the Object class may to a too harsh requirement for\nSignal usage in applications, keep the existing behaviour working if the\nslot doesn't inherit from the Object class. We may reconsider this later\nand require all slot objects to inherit from the Object class.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n Documentation/Doxyfile.in | 4 +-\n include/libcamera/meson.build | 1 +\n include/libcamera/object.h | 35 +++++++++++\n include/libcamera/signal.h | 115 +++++++++++++++++++++++-----------\n src/libcamera/meson.build | 1 +\n src/libcamera/object.cpp | 50 +++++++++++++++\n src/libcamera/signal.cpp | 5 ++\n test/signal.cpp | 37 +++++++++++\n 8 files changed, 211 insertions(+), 37 deletions(-)\n create mode 100644 include/libcamera/object.h\n create mode 100644 src/libcamera/object.cpp", "diff": "diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\nindex b78fb3a0b0b6..3e2b7fd9da0e 100644\n--- a/Documentation/Doxyfile.in\n+++ b/Documentation/Doxyfile.in\n@@ -860,7 +860,9 @@ EXCLUDE_PATTERNS =\n # Note that the wildcards are matched against the file with absolute path, so to\n # exclude all test directories use the pattern */test/*\n \n-EXCLUDE_SYMBOLS = libcamera::SlotBase \\\n+EXCLUDE_SYMBOLS = libcamera::SignalBase \\\n+ libcamera::SlotArgs \\\n+ libcamera::SlotBase \\\n libcamera::SlotMember \\\n libcamera::SlotStatic\n \ndiff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\nindex 5788e9bbdf3e..3f4d1e28208b 100644\n--- a/include/libcamera/meson.build\n+++ b/include/libcamera/meson.build\n@@ -5,6 +5,7 @@ libcamera_api = files([\n 'event_dispatcher.h',\n 'event_notifier.h',\n 'libcamera.h',\n+ 'object.h',\n 'request.h',\n 'signal.h',\n 'stream.h',\ndiff --git a/include/libcamera/object.h b/include/libcamera/object.h\nnew file mode 100644\nindex 000000000000..eadd41f9a41f\n--- /dev/null\n+++ b/include/libcamera/object.h\n@@ -0,0 +1,35 @@\n+/* SPDX-License-Identifier: LGPL-2.1-or-later */\n+/*\n+ * Copyright (C) 2019, Google Inc.\n+ *\n+ * object.h - Base object\n+ */\n+#ifndef __LIBCAMERA_OBJECT_H__\n+#define __LIBCAMERA_OBJECT_H__\n+\n+#include <list>\n+\n+namespace libcamera {\n+\n+class SignalBase;\n+template<typename... Args>\n+class Signal;\n+\n+class Object\n+{\n+public:\n+\tvirtual ~Object();\n+\n+private:\n+\ttemplate<typename... Args>\n+\tfriend class Signal;\n+\n+\tvoid connect(SignalBase *signal);\n+\tvoid disconnect(SignalBase *signal);\n+\n+\tstd::list<SignalBase *> signals_;\n+};\n+\n+}; /* namespace libcamera */\n+\n+#endif /* __LIBCAMERA_OBJECT_H__ */\ndiff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\nindex 85dc481d6957..434fc0109f42 100644\n--- a/include/libcamera/signal.h\n+++ b/include/libcamera/signal.h\n@@ -10,32 +10,47 @@\n #include <list>\n #include <vector>\n \n+#include <libcamera/object.h>\n+\n namespace libcamera {\n \n template<typename... Args>\n class Signal;\n \n-template<typename... Args>\n class SlotBase\n {\n public:\n-\tSlotBase(void *obj)\n-\t\t: obj_(obj) { }\n-\tvirtual ~SlotBase() { }\n+\tSlotBase(void *obj, bool isObject)\n+\t\t: obj_(obj), isObject_(isObject) {}\n+\tvirtual ~SlotBase() {}\n \n-\tvirtual void invoke(Args... args) = 0;\n+\tvoid *obj() { return obj_; }\n+\tbool isObject() const { return isObject_; }\n \n protected:\n-\tfriend class Signal<Args...>;\n \tvoid *obj_;\n+\tbool isObject_;\n+};\n+\n+template<typename... Args>\n+class SlotArgs : public SlotBase\n+{\n+public:\n+\tSlotArgs(void *obj, bool isObject)\n+\t\t: SlotBase(obj, isObject) {}\n+\n+\tvirtual void invoke(Args... args) = 0;\n+\n+protected:\n+\tfriend class Signal<Args...>;\n };\n \n template<typename T, typename... Args>\n-class SlotMember : public SlotBase<Args...>\n+class SlotMember : public SlotArgs<Args...>\n {\n public:\n-\tSlotMember(T *obj, void (T::*func)(Args...))\n-\t\t: SlotBase<Args...>(obj), func_(func) { }\n+\tSlotMember(T *obj, bool isObject, void (T::*func)(Args...))\n+\t\t: SlotArgs<Args...>(obj, isObject), func_(func) {}\n \n \tvoid invoke(Args... args) { (static_cast<T *>(this->obj_)->*func_)(args...); }\n \n@@ -45,11 +60,11 @@ private:\n };\n \n template<typename... Args>\n-class SlotStatic : public SlotBase<Args...>\n+class SlotStatic : public SlotArgs<Args...>\n {\n public:\n \tSlotStatic(void (*func)(Args...))\n-\t\t: SlotBase<Args...>(nullptr), func_(func) { }\n+\t\t: SlotArgs<Args...>(nullptr, false), func_(func) {}\n \n \tvoid invoke(Args... args) { (*func_)(args...); }\n \n@@ -58,21 +73,57 @@ private:\n \tvoid (*func_)(Args...);\n };\n \n+class SignalBase\n+{\n+public:\n+\ttemplate<typename T>\n+\tvoid disconnect(T *object)\n+\t{\n+\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n+\t\t\tSlotBase *slot = *iter;\n+\t\t\tif (slot->obj() == object) {\n+\t\t\t\titer = slots_.erase(iter);\n+\t\t\t\tdelete slot;\n+\t\t\t} else {\n+\t\t\t\t++iter;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+protected:\n+\tfriend class Object;\n+\tstd::list<SlotBase *> slots_;\n+};\n+\n template<typename... Args>\n-class Signal\n+class Signal : public SignalBase\n {\n public:\n \tSignal() { }\n \t~Signal()\n \t{\n-\t\tfor (SlotBase<Args...> *slot : slots_)\n+\t\tfor (SlotBase *slot : slots_) {\n+\t\t\tif (slot->isObject())\n+\t\t\t\tstatic_cast<Object *>(slot->obj())->disconnect(this);\n \t\t\tdelete slot;\n+\t\t}\n \t}\n \n+#ifndef __DOXYGEN__\n+\ttemplate<typename T, typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>\n+\tvoid connect(T *object, void (T::*func)(Args...))\n+\t{\n+\t\tobject->connect(this);\n+\t\tslots_.push_back(new SlotMember<T, Args...>(object, true, func));\n+\t}\n+\n+\ttemplate<typename T, typename std::enable_if<!std::is_base_of<Object, T>::value>::type * = nullptr>\n+#else\n \ttemplate<typename T>\n+#endif\n \tvoid connect(T *object, void (T::*func)(Args...))\n \t{\n-\t\tslots_.push_back(new SlotMember<T, Args...>(object, func));\n+\t\tslots_.push_back(new SlotMember<T, Args...>(object, false, func));\n \t}\n \n \tvoid connect(void (*func)(Args...))\n@@ -82,7 +133,7 @@ public:\n \n \tvoid disconnect()\n \t{\n-\t\tfor (SlotBase<Args...> *slot : slots_)\n+\t\tfor (SlotBase *slot : slots_)\n \t\t\tdelete slot;\n \t\tslots_.clear();\n \t}\n@@ -90,27 +141,21 @@ public:\n \ttemplate<typename T>\n \tvoid disconnect(T *object)\n \t{\n-\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n-\t\t\tSlotBase<Args...> *slot = *iter;\n-\t\t\tif (slot->obj_ == object) {\n-\t\t\t\titer = slots_.erase(iter);\n-\t\t\t\tdelete slot;\n-\t\t\t} else {\n-\t\t\t\t++iter;\n-\t\t\t}\n-\t\t}\n+\t\tSignalBase::disconnect(object);\n \t}\n \n \ttemplate<typename T>\n \tvoid disconnect(T *object, void (T::*func)(Args...))\n \t{\n \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n-\t\t\tSlotBase<Args...> *slot = *iter;\n+\t\t\tSlotArgs<Args...> *slot = static_cast<SlotArgs<Args...> *>(*iter);\n \t\t\t/*\n-\t\t\t * If the obj_ pointer matches the object types must\n-\t\t\t * match, so we can safely cast to SlotMember<T, Args>.\n+\t\t\t * If the obj() pointer matches the object, the slot is\n+\t\t\t * guaranteed to be a member slot, so we can safely\n+\t\t\t * cast it to SlotMember<T, Args...> and access its\n+\t\t\t * func_ member.\n \t\t\t */\n-\t\t\tif (slot->obj_ == object &&\n+\t\t\tif (slot->obj() == object &&\n \t\t\t static_cast<SlotMember<T, Args...> *>(slot)->func_ == func) {\n \t\t\t\titer = slots_.erase(iter);\n \t\t\t\tdelete slot;\n@@ -123,8 +168,8 @@ public:\n \tvoid disconnect(void (*func)(Args...))\n \t{\n \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n-\t\t\tSlotBase<Args...> *slot = *iter;\n-\t\t\tif (slot->obj_ == nullptr &&\n+\t\t\tSlotArgs<Args...> *slot = *iter;\n+\t\t\tif (slot->obj() == nullptr &&\n \t\t\t static_cast<SlotStatic<Args...> *>(slot)->func_ == func) {\n \t\t\t\titer = slots_.erase(iter);\n \t\t\t\tdelete slot;\n@@ -140,13 +185,11 @@ public:\n \t\t * Make a copy of the slots list as the slot could call the\n \t\t * disconnect operation, invalidating the iterator.\n \t\t */\n-\t\tstd::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };\n-\t\tfor (SlotBase<Args...> *slot : slots)\n-\t\t\tslot->invoke(args...);\n+\t\tstd::vector<SlotBase *> slots{ slots_.begin(), slots_.end() };\n+\t\tfor (SlotBase *slot : slots) {\n+\t\t\tstatic_cast<SlotArgs<Args...> *>(slot)->invoke(args...);\n+\t\t}\n \t}\n-\n-private:\n-\tstd::list<SlotBase<Args...> *> slots_;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\nindex c5354c136563..8384cd0af451 100644\n--- a/src/libcamera/meson.build\n+++ b/src/libcamera/meson.build\n@@ -10,6 +10,7 @@ libcamera_sources = files([\n 'log.cpp',\n 'media_device.cpp',\n 'media_object.cpp',\n+ 'object.cpp',\n 'pipeline_handler.cpp',\n 'request.cpp',\n 'signal.cpp',\ndiff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\nnew file mode 100644\nindex 000000000000..826eed6f9b3a\n--- /dev/null\n+++ b/src/libcamera/object.cpp\n@@ -0,0 +1,50 @@\n+/* SPDX-License-Identifier: LGPL-2.1-or-later */\n+/*\n+ * Copyright (C) 2019, Google Inc.\n+ *\n+ * object.cpp - Base object\n+ */\n+\n+#include <libcamera/object.h>\n+#include <libcamera/signal.h>\n+\n+/**\n+ * \\file object.h\n+ * \\brief Base object to support automatic signal disconnection\n+ */\n+\n+namespace libcamera {\n+\n+/**\n+ * \\class Object\n+ * \\brief Base object to support automatic signal disconnection\n+ *\n+ * The Object class simplifies signal/slot handling for classes implementing\n+ * slots. By inheriting from Object, an object is automatically disconnected\n+ * from all connected signals when it gets destroyed.\n+ *\n+ * \\sa Signal\n+ */\n+\n+Object::~Object()\n+{\n+\tfor (SignalBase *signal : signals_)\n+\t\tsignal->disconnect(this);\n+}\n+\n+void Object::connect(SignalBase *signal)\n+{\n+\tsignals_.push_back(signal);\n+}\n+\n+void Object::disconnect(SignalBase *signal)\n+{\n+\tfor (auto iter = signals_.begin(); iter != signals_.end(); ) {\n+\t\tif (*iter == signal)\n+\t\t\titer = signals_.erase(iter);\n+\t\telse\n+\t\t\titer++;\n+\t}\n+}\n+\n+}; /* namespace libcamera */\ndiff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\nindex 8d62b5beeeac..cb7daa11dab1 100644\n--- a/src/libcamera/signal.cpp\n+++ b/src/libcamera/signal.cpp\n@@ -47,6 +47,11 @@ namespace libcamera {\n * \\brief Connect the signal to a member function slot\n * \\param object The slot object pointer\n * \\param func The slot member function\n+ *\n+ * If the typename T inherits from Object, the signal will be automatically\n+ * disconnected from the \\a func slot of \\a object when \\a object is destroyed.\n+ * Otherwise the caller shall disconnect signals manually before destroying \\a\n+ * object.\n */\n \n /**\ndiff --git a/test/signal.cpp b/test/signal.cpp\nindex ab69ca1b663d..19a52c603a4a 100644\n--- a/test/signal.cpp\n+++ b/test/signal.cpp\n@@ -8,6 +8,7 @@\n #include <iostream>\n #include <string.h>\n \n+#include <libcamera/object.h>\n #include <libcamera/signal.h>\n \n #include \"test.h\"\n@@ -22,6 +23,15 @@ static void slotStatic(int value)\n \tvalueStatic_ = value;\n }\n \n+class SlotObject : public Object\n+{\n+public:\n+\tvoid slot()\n+\t{\n+\t\tvalueStatic_ = 1;\n+\t}\n+};\n+\n class SignalTest : public Test\n {\n protected:\n@@ -139,6 +149,33 @@ protected:\n \t\t\treturn TestFail;\n \t\t}\n \n+\t\t/*\n+\t\t * Test automatic disconnection on object deletion. Connect the\n+\t\t * slot twice to ensure all instances are disconnected.\n+\t\t */\n+\t\tsignalVoid_.disconnect();\n+\n+\t\tSlotObject *slotObject = new SlotObject();\n+\t\tsignalVoid_.connect(slotObject, &SlotObject::slot);\n+\t\tsignalVoid_.connect(slotObject, &SlotObject::slot);\n+\t\tdelete slotObject;\n+\t\tvalueStatic_ = 0;\n+\t\tsignalVoid_.emit();\n+\t\tif (valueStatic_ != 0) {\n+\t\t\tcout << \"Signal disconnection on object deletion test failed\" << endl;\n+\t\t\treturn TestFail;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Test that signal deletion disconnects objects. This shall\n+\t\t * not generate any valgrind warning.\n+\t\t */\n+\t\tSignal<> *signal = new Signal<>();\n+\t\tslotObject = new SlotObject();\n+\t\tsignal->connect(slotObject, &SlotObject::slot);\n+\t\tdelete signal;\n+\t\tdelete slotObject;\n+\n \t\treturn TestPass;\n \t}\n \n", "prefixes": [ "libcamera-devel", "3/3" ] }