search

farsightsec / fstrm

Frame Streams implementation in C
MIT License
59 stars 27 forks source link

Branches/issue34 #35

Closed cmikk closed 7 years ago

cmikk commented 7 years ago

Check for the existence of pthread_condattr_setclock in addition to clock_gettime.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 72.978% when pulling bd5cc24e5903bf539e0ab6db11827fa8653aac6a on branches/issue34 into 9db2516040a6544b2e8f1127c42e8cbb415adcbf on next.

edmonds commented 7 years ago

I'm kinda confused by this. Why should the use of clock_gettime be conditional on pthread_condattr_setclock? That prevents any non- pthread_cond_timedwait() related uses of clock_gettime, e.g. in fstrm__iothr_maybe_open().

cmikk commented 7 years ago

@edmonds - I had missed those uses of clock_gettime() in my review.

The core issue is that HAVE_CLOCK_GETTIME presumes pthread_condattr_setclock() exists, which is not necessarily the case. I've pushed a more correct fix to the branch head.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 73.226% when pulling 2ebca25deaf89e0d36855875d9f8628ab2fda640 on branches/issue34 into 9db2516040a6544b2e8f1127c42e8cbb415adcbf on next.

edmonds commented 7 years ago

@cmikk I think the #ifdef in fstrm__iothr_thr() also needs an && HAVE_PTHREAD_CONDATTR_SETCLOCK?

https://github.com/farsightsec/fstrm/blob/v0.3.1/fstrm/iothr.c#L614-L623

cmikk commented 7 years ago

Agreed, and fixed.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 73.226% when pulling 620cd7efbd08107beb5a7685b41fda2a8cfe8bd9 on branches/issue34 into 9db2516040a6544b2e8f1127c42e8cbb415adcbf on next.

reedjc commented 7 years ago

AC_CHECK_FUNCS([clock_gettime, pthread_condattr_setclock])

is broken. See the comma in the functions list. The configure output shows it too and the config.out shows errors like 

#undef clock_gettime,
                    ^
                    //
conftest.c:52:22: error: expected identifier or '('
coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 73.226% when pulling 556991cb3ddbad5f14cc9296e9403b97f68e4fbd on branches/issue34 into 9db2516040a6544b2e8f1127c42e8cbb415adcbf on next.

cmikk commented 7 years ago

The typo @reedjc pointed out made the builds (and checks) work by disabling clock_gettime() entirely. Correcting the typo exposed an error in the #ifdef fix, which required a different choice of default clock for clock_gettime().

My research indicated that if pthread_condattr_setclock() is not available to say otherwise, pthread_cond_timedwait() uses CLOCK_REALTIME. I amended the #ifdef fix to use CLOCK_REALTIME for deadline computation if clock_gettime() is available but pthread_condattr_setclock() is not, squashed in a fix to the erroneous ',' in configure.ac, and pushed a clean branch to replace the old branches/issue34.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 72.978% when pulling 206894cfaac4b69134b339d22ca9da12f1854210 on branches/issue34 into 9db2516040a6544b2e8f1127c42e8cbb415adcbf on next.