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

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

> I think something with "posix" in the name would be good as it only affects posix mutexes?

Whether a mutex is "posix" or not is a characteristic of the target being built for, not the kind of mutex being created. There are no posix mutexes for our Windows or VxWorks implementations so that makes no sense to me. I suggested using a more pthreads-style name osd_mutex_init() because the thing it returns is an actual pthread_mutex_t which gets passed into other pthread_*() routines. Passing something apparently created by a non-pthreads routine into a pthreads routine is a bit of a code smell, it looks wrong even though it isn't really.

> You mean expected failure is success? How to program that?

There are some problems with the test program; it always defines TEST_PLAN as 8, but the number of test results that will be reported will not be the same for all builds. There are several tests which won't report any result if HAVE_CPU_AFFINITY is undefined. This situation is why the unitTest API provides testSkip(), which lets a program declare that it can't run some number of tests in its current configuration or on this target. That is how running the same test with both schedulers would work, at run-time the code detects whether the tests should pass or not and calls testSkip() instead if not. Please don't try to adjust the test plan at runtime instead, it's much easier to read test code if a particular group of tests always generates the same number of results.

My suggestion of running the thread test programs a second time on Linux was really an idea for Michael and Ralph to consider: On the CI runners that give us sudo it ought to be possible to configure our account's maxrtprio hard limit somewhere in the job setup, then we would add linux-only scripts such as epicsThreadPriorityRT.plt which run the program again using say
        prlimit -r 80 ./epicsThreadPriority

« Back to merge proposal