On 07/22/2014 03:59 PM, Frank Ch. Eigler wrote:
Hi, Dave -
The following commit implements a global timeout for
__pmDiscoverServicesWithOptions().
Looks good.
Thanks for reviewing.
The implementation is a sleeping thread which wakes up after the
specified amount of time and sets a flag indicating that the timeout
has expired.
That sounds good. The lack of fallback for non-threaded libpcp's
could be a problem; have you considered returning an outright error
instead of the "__pmNotifyErr() & return OK" in this case?
I didn't consider this. Probably more flexible for the API users who
would otherwise have no indication that the timeout was ignored.
In __pmDiscoverServicesWithOptions(), during the thread cleanup,
it looks like the code cares about the sts of the pthread_cancel(),
but doesn't really. -ESRCH is a valid non-error case there, FWIW.
Consider just a "(void) pthread_cancel(...);".
Yeah. sts was set but actually ignored. I'll just remove the assignment.
For the __pmServiceDiscoveryOptions timedOut field, did you consider
having the thread set the INTERRUPTED bit in the flags, instead of
that new field? If so, good job figuring out that it'd be a bad idea :-)
due to the need to eschew having libpcp modify that const * flags.
Well no, because we had already discussed it in context of changing the
flags to const volatile *.
(All that C options-processing string/realloc() stuff in pmfind.c must
leave one, in weak moments, covetous of C++.)
Indeed :-(
Function overloading would also be nice for extending the API.
Dave
|