Code review comment for ~dirk.zimoch/epics-base:epicsMutexPriorityInheritance

Revision history for this message
Andrew Johnson (anj) wrote :

Names beginning with epics are generally intended for public use, which the epicsPosixMutex stuff shouldn't be. If I've understood the code correctly you aren't replacing the pthread_mutex_t type but are providing a wrapper for pthread_mutex_init() that creates one, and you're also moving the need for the caller to set mutex attributes on the new mutex. Any new publicly visible symbols that are meant to be internal to a libCom implementation should begin with "osd", so maybe osd_mutex_init()?

I've read the essay in the comments just below epicsMutexOsdShow(), but I'm not sure how it helps future maintainers to understand this file. The discussion there doesn't seem appropriate in source code comments.

Is the new test program generic? It has some CPU-affinity stuff in it which is conditional on glibc; what happens on other OSs (Windows, MacOS, RTEMS-5, embedded Linux) that can also do SMP and don't use glibc?

I'm not sure I like the new environment variable or understand what effect not defining it will have. Is there a good reason to continue to support the previous mode if in practice it's broken? As long as we document the changes in behavior when SCHED_FIFO is available (I'm assuming nothing should change when it isn't) I'm not convinced we need the env switch, which would resolve Dirk's question about how to set an environment variable before running a test program.

However I wonder if it would be desirable to run this new test (and maybe some of the other thread-related test programs too) twice, once with the default scheduler and once under SCHED_FIFO?

Finally I think we would probably want to delay merging this until after Heinz' RTEMS-5 code has been pulled in, that might require some changes to this MR because he uses the Posix APIs in the new RTEMS port.

review: Needs Fixing

« Back to merge proposal