fly316/PX4-Autopilot
7
0
Fork 0
mirror of https://gitee.com/mirrors_PX4/PX4-Autopilot.git synced 2026-05-17 00:37:35 +08:00
Code Issues Packages Projects Releases Wiki Activity

orb: rm static from DeviceMaster::_node_map & use the non-static getDeviceNode in uORB::Manager

Browse Source 
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
This commit is contained in:
Beat Küng
2016-04-29 13:14:29 +02:00
committed by Lorenz Meier
parent 45a0a7c5ab
commit 7280f71cef
5 changed files with 68 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/modules/uORB/uORBDevices_nuttx.cpp
+12 -4
View File
@@ -43,8 +43,6 @@
#include <px4_sem.hpp>
#include <stdlib.h>
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;
src/modules/uORB/uORBDevices_nuttx.hpp
+18 -4
View File
@@ -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;
};
src/modules/uORB/uORBDevices_posix.cpp
+12 -4
View File
@@ -44,8 +44,6 @@
#include <px4_sem.hpp>
#include <stdlib.h>
std::map<std::string, uORB::DeviceNode *> 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);
src/modules/uORB/uORBDevices_posix.hpp
+17 -4
View File
@@ -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<std::string, uORB::DeviceNode *> _node_map;
std::map<std::string, uORB::DeviceNode *> _node_map;
};
#endif /* _uORBDeviceNode_posix.hpp */
src/modules/uORB/uORBManager.cpp
+9 -7
View File
@@ -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) {