From 7280f71cefc3e3ab36add91e239c70d15394d21d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beat=20K=C3=BCng?= Date: Fri, 29 Apr 2016 13:14:29 +0200 Subject: [PATCH] orb: rm static from DeviceMaster::_node_map & use the non-static getDeviceNode in uORB::Manager Reasons: - DeviceMaster::_node_map does not need to be shared among instances, because there is at most 1 instance per Flavor and different Flavors have non-intersecting device paths. - Keeping it static would also require a static lock - DeviceMaster::_node_map was not locked at all when used from uORB::Manager So this fixes two synchronization issues: - Different DeviceMaster objects could access the same static data in parallel - getDeviceNode() called from uORB::Manager did not use any locking at all --- src/modules/uORB/uORBDevices_nuttx.cpp | 16 ++++++++++++---- src/modules/uORB/uORBDevices_nuttx.hpp | 22 ++++++++++++++++++---- src/modules/uORB/uORBDevices_posix.cpp | 16 ++++++++++++---- src/modules/uORB/uORBDevices_posix.hpp | 21 +++++++++++++++++---- src/modules/uORB/uORBManager.cpp | 16 +++++++++------- 5 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/modules/uORB/uORBDevices_nuttx.cpp b/src/modules/uORB/uORBDevices_nuttx.cpp index 93eeaa8d92..9b2780fb25 100644 --- a/src/modules/uORB/uORBDevices_nuttx.cpp +++ b/src/modules/uORB/uORBDevices_nuttx.cpp @@ -43,8 +43,6 @@ #include #include -uORB::ORBMap uORB::DeviceMaster::_node_map; - uORB::DeviceNode::DeviceNode ( const struct orb_metadata *meta, @@ -737,7 +735,7 @@ uORB::DeviceMaster::ioctl(struct file *filp, int cmd, unsigned long arg) if (ret == -EEXIST) { /* if the node exists already, get the existing one and check if * something has been published yet. */ - uORB::DeviceNode *existing_node = GetDeviceNode(devpath); + uORB::DeviceNode *existing_node = getDeviceNodeLocked(devpath); if ((existing_node != nullptr) && !(existing_node->is_published())) { /* nothing has been published yet, lets claim it */ @@ -773,7 +771,17 @@ uORB::DeviceMaster::ioctl(struct file *filp, int cmd, unsigned long arg) } } -uORB::DeviceNode *uORB::DeviceMaster::GetDeviceNode(const char *nodepath) +uORB::DeviceNode *uORB::DeviceMaster::getDeviceNode(const char *nodepath) +{ + lock(); + uORB::DeviceNode *node = getDeviceNodeLocked(nodepath); + unlock(); + //We can safely return the node that can be used by any thread, because + //a DeviceNode never gets deleted. + return node; +} + +uORB::DeviceNode *uORB::DeviceMaster::getDeviceNodeLocked(const char *nodepath) { uORB::DeviceNode *rc = nullptr; diff --git a/src/modules/uORB/uORBDevices_nuttx.hpp b/src/modules/uORB/uORBDevices_nuttx.hpp index 2a92a5ad16..9d30be2a52 100644 --- a/src/modules/uORB/uORBDevices_nuttx.hpp +++ b/src/modules/uORB/uORBDevices_nuttx.hpp @@ -257,16 +257,30 @@ private: // private class methods. class uORB::DeviceMaster : public device::CDev { public: + virtual int ioctl(struct file *filp, int cmd, unsigned long arg); + + /** + * Public interface for getDeviceNodeLocked(). Takes care of synchronization. + * @return node if exists, nullptr otherwise + */ + uORB::DeviceNode *getDeviceNode(const char *node_name); + +private: + // Private constructor, uORB::Manager takes care of its creation DeviceMaster(Flavor f); virtual ~DeviceMaster(); friend class uORB::Manager; - static uORB::DeviceNode *GetDeviceNode(const char *node_name); - virtual int ioctl(struct file *filp, int cmd, unsigned long arg); -private: + /** + * Find a node give its name. + * _lock must already be held when calling this. + * @return node if exists, nullptr otherwise + */ + uORB::DeviceNode *getDeviceNodeLocked(const char *node_name); + const Flavor _flavor; - static ORBMap _node_map; + ORBMap _node_map; }; diff --git a/src/modules/uORB/uORBDevices_posix.cpp b/src/modules/uORB/uORBDevices_posix.cpp index a7c7d3cfe6..d61a887949 100644 --- a/src/modules/uORB/uORBDevices_posix.cpp +++ b/src/modules/uORB/uORBDevices_posix.cpp @@ -44,8 +44,6 @@ #include #include -std::map uORB::DeviceMaster::_node_map; - uORB::DeviceNode::SubscriberData *uORB::DeviceNode::filp_to_sd(device::file_t *filp) { @@ -742,7 +740,7 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg) if (ret == -EEXIST) { /* if the node exists already, get the existing one and check if * something has been published yet. */ - uORB::DeviceNode *existing_node = GetDeviceNode(devpath); + uORB::DeviceNode *existing_node = getDeviceNodeLocked(devpath); if ((existing_node != nullptr) && !(existing_node->is_published())) { /* nothing has been published yet, lets claim it */ @@ -779,7 +777,17 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg) } } -uORB::DeviceNode *uORB::DeviceMaster::GetDeviceNode(const char *nodepath) +uORB::DeviceNode *uORB::DeviceMaster::getDeviceNode(const char *nodepath) +{ + lock(); + uORB::DeviceNode *node = getDeviceNodeLocked(nodepath); + unlock(); + //We can safely return the node that can be used by any thread, because + //a DeviceNode never gets deleted. + return node; +} + +uORB::DeviceNode *uORB::DeviceMaster::getDeviceNodeLocked(const char *nodepath) { uORB::DeviceNode *rc = nullptr; std::string np(nodepath); diff --git a/src/modules/uORB/uORBDevices_posix.hpp b/src/modules/uORB/uORBDevices_posix.hpp index a129bd7905..bcfbc41951 100644 --- a/src/modules/uORB/uORBDevices_posix.hpp +++ b/src/modules/uORB/uORBDevices_posix.hpp @@ -191,17 +191,30 @@ private: class uORB::DeviceMaster : public device::VDev { public: + virtual int ioctl(device::file_t *filp, int cmd, unsigned long arg); + + /** + * Public interface for getDeviceNodeLocked(). Takes care of synchronization. + * @return node if exists, nullptr otherwise + */ + uORB::DeviceNode *getDeviceNode(const char *node_name); + +private: + // Private constructor, uORB::Manager takes care of its creation DeviceMaster(Flavor f); virtual ~DeviceMaster(); friend class uORB::Manager; - static uORB::DeviceNode *GetDeviceNode(const char *node_name); + /** + * Find a node give its name. + * _lock must already be held when calling this. + * @return node if exists, nullptr otherwise + */ + uORB::DeviceNode *getDeviceNodeLocked(const char *node_name); - virtual int ioctl(device::file_t *filp, int cmd, unsigned long arg); -private: const Flavor _flavor; - static std::map _node_map; + std::map _node_map; }; #endif /* _uORBDeviceNode_posix.hpp */ diff --git a/src/modules/uORB/uORBManager.cpp b/src/modules/uORB/uORBManager.cpp index b73dcb6e43..4e6a0a81df 100644 --- a/src/modules/uORB/uORBManager.cpp +++ b/src/modules/uORB/uORBManager.cpp @@ -450,10 +450,10 @@ int16_t uORB::Manager::process_add_subscription(const char *messageName, _remote_subscriber_topics.insert(messageName); char nodepath[orb_maxpath]; int ret = uORB::Utils::node_mkpath(nodepath, PUBSUB, messageName); + DeviceMaster *device_master = get_device_master(PUBSUB); - if (ret == OK) { - // get the node name. - uORB::DeviceNode *node = uORB::DeviceMaster::GetDeviceNode(nodepath); + if (ret == OK && device_master) { + uORB::DeviceNode *node = device_master->getDeviceNode(nodepath); if (node == nullptr) { PX4_DEBUG("[posix-uORB::Manager::process_add_subscription(%d)]DeviceNode(%s) not created yet", @@ -482,9 +482,10 @@ int16_t uORB::Manager::process_remove_subscription( _remote_subscriber_topics.erase(messageName); char nodepath[orb_maxpath]; int ret = uORB::Utils::node_mkpath(nodepath, PUBSUB, messageName); + DeviceMaster *device_master = get_device_master(PUBSUB); - if (ret == OK) { - uORB::DeviceNode *node = uORB::DeviceMaster::GetDeviceNode(nodepath); + if (ret == OK && device_master) { + uORB::DeviceNode *node = device_master->getDeviceNode(nodepath); // get the node name. if (node == nullptr) { @@ -511,9 +512,10 @@ int16_t uORB::Manager::process_received_message(const char *messageName, int16_t rc = -1; char nodepath[orb_maxpath]; int ret = uORB::Utils::node_mkpath(nodepath, PUBSUB, messageName); + DeviceMaster *device_master = get_device_master(PUBSUB); - if (ret == OK) { - uORB::DeviceNode *node = uORB::DeviceMaster::GetDeviceNode(nodepath); + if (ret == OK && device_master) { + uORB::DeviceNode *node = device_master->getDeviceNode(nodepath); // get the node name. if (node == nullptr) {