orb posix: fix multi-threading issues

in detail:
- in the write method: the following are not necessarily atomic operations:
 	_last_update = hrt_absolute_time();
 	_generation++;
- appears_updated() was called with a lock held in some cases, but not
  in ioctl
- use the SmartLock class, so that unlock() is not needed before every
  return call. Makes it less error prone
This commit is contained in:
Beat Küng 2016-04-25 17:00:58 +02:00 committed by Lorenz Meier
parent 9a0cff2a00
commit 44012be8b6
1 changed files with 16 additions and 20 deletions

View File

@ -253,20 +253,20 @@ uORB::DeviceNode::write(device::file_t *filp, const char *buffer, size_t buflen)
return -EIO; return -EIO;
} }
/* Perform an atomic copy. */
lock(); lock();
memcpy(_data, buffer, _meta->o_size); memcpy(_data, buffer, _meta->o_size);
unlock();
/* update the timestamp and generation count */ /* update the timestamp and generation count */
_last_update = hrt_absolute_time(); _last_update = hrt_absolute_time();
_generation++; _generation++;
_published = true;
unlock();
/* notify any poll waiters */ /* notify any poll waiters */
poll_notify(POLLIN); poll_notify(POLLIN);
_published = true;
return _meta->o_size; return _meta->o_size;
} }
@ -278,16 +278,22 @@ uORB::DeviceNode::ioctl(device::file_t *filp, int cmd, unsigned long arg)
switch (cmd) { switch (cmd) {
case ORBIOCLASTUPDATE: case ORBIOCLASTUPDATE:
lock();
*(hrt_abstime *)arg = _last_update; *(hrt_abstime *)arg = _last_update;
unlock();
return PX4_OK; return PX4_OK;
case ORBIOCUPDATED: case ORBIOCUPDATED:
lock();
*(bool *)arg = appears_updated(sd); *(bool *)arg = appears_updated(sd);
unlock();
return PX4_OK; return PX4_OK;
case ORBIOCSETINTERVAL: case ORBIOCSETINTERVAL:
lock();
sd->update_interval = arg; sd->update_interval = arg;
sd->last_update = hrt_absolute_time(); sd->last_update = hrt_absolute_time();
unlock();
return PX4_OK; return PX4_OK;
case ORBIOCGADVERTISER: case ORBIOCGADVERTISER:
@ -409,6 +415,7 @@ uORB::DeviceNode::poll_notify_one(px4_pollfd_struct_t *fds, pollevent_t events)
bool bool
uORB::DeviceNode::appears_updated(SubscriberData *sd) uORB::DeviceNode::appears_updated(SubscriberData *sd)
{ {
/* block if in simulation mode */ /* block if in simulation mode */
while (px4_sim_delay_enabled()) { while (px4_sim_delay_enabled()) {
usleep(100); usleep(100);
@ -420,8 +427,7 @@ uORB::DeviceNode::appears_updated(SubscriberData *sd)
/* check if this topic has been published yet, if not bail out */ /* check if this topic has been published yet, if not bail out */
if (_data == nullptr) { if (_data == nullptr) {
ret = false; return false;
goto out;
} }
/* /*
@ -467,8 +473,6 @@ uORB::DeviceNode::appears_updated(SubscriberData *sd)
break; break;
} }
out:
/* consider it updated */
return ret; return ret;
} }
@ -535,14 +539,14 @@ int16_t uORB::DeviceNode::process_add_subscription(int32_t rateInHz)
ch->send_message(_meta->o_name, _meta->o_size, _data); ch->send_message(_meta->o_name, _meta->o_size, _data);
} }
return 0; return PX4_OK;
} }
//----------------------------------------------------------------------------- //-----------------------------------------------------------------------------
//----------------------------------------------------------------------------- //-----------------------------------------------------------------------------
int16_t uORB::DeviceNode::process_remove_subscription() int16_t uORB::DeviceNode::process_remove_subscription()
{ {
return 0; return PX4_OK;
} }
//----------------------------------------------------------------------------- //-----------------------------------------------------------------------------
@ -608,9 +612,6 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg)
return ret; return ret;
} }
/* ensure that only one advertiser runs through this critical section */
lock();
ret = ERROR; ret = ERROR;
/* try for topic groups */ /* try for topic groups */
@ -624,11 +625,12 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg)
group_tries = *adv->instance; group_tries = *adv->instance;
if (group_tries >= max_group_tries) { if (group_tries >= max_group_tries) {
unlock();
return -ENOMEM; return -ENOMEM;
} }
} }
SmartLock smart_lock(*this);
do { do {
/* if path is modifyable change try index */ /* if path is modifyable change try index */
if (adv->instance != nullptr) { if (adv->instance != nullptr) {
@ -641,7 +643,6 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg)
objname = strdup(meta->o_name); objname = strdup(meta->o_name);
if (objname == nullptr) { if (objname == nullptr) {
unlock();
return -ENOMEM; return -ENOMEM;
} }
@ -649,7 +650,6 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg)
devpath = strdup(nodepath); devpath = strdup(nodepath);
if (devpath == nullptr) { if (devpath == nullptr) {
unlock();
free((void *)objname); free((void *)objname);
return -ENOMEM; return -ENOMEM;
} }
@ -659,7 +659,6 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg)
/* if we didn't get a device, that's bad */ /* if we didn't get a device, that's bad */
if (node == nullptr) { if (node == nullptr) {
unlock();
free((void *)objname); free((void *)objname);
free((void *)devpath); free((void *)devpath);
return -ENOMEM; return -ENOMEM;
@ -704,9 +703,6 @@ uORB::DeviceMaster::ioctl(device::file_t *filp, int cmd, unsigned long arg)
ret = -ENOMEM; ret = -ENOMEM;
} }
/* the file handle for the driver has been created, unlock */
unlock();
return ret; return ret;
} }