Thank you for reviewing this. On Mon, 2008-01-28 at 21:10 +0100, Paolo Molaro wrote: > On 01/28/08 Jonathan Pryor wrote: > > > It is important (as in my initialy API sketch) that this function take > > > the signal_info and not the dignal number, as this implementation allows > > > only just an handler per signal in the API. > > > > This has been corrected. We now support up to 64 signal handlers within > > a process, and the same signal can be registered multiple times; each > > such registration gets a different index, and is thus independent. > > Deregistration is handled incorrectly: if there are two handlers for the > same signal it gets disabled at the first uninstall.
Not necessarily; it depends on the order of unregistration (if the 2nd UnixSignal instance is destroyed before the first one is, things will work fine). Regardless, this is a bug that should be fixed. > > The only other major change is the addition of a > > UnixSignal.WaitAny(UnixSignal[]) method, which allows waiting on more > > than one handle (since WaitHandle.WaitAny() can't currently be used with > > WaitHandle subclasses like UnixSignal). > > I suggest using params in the array argument. I'm not sure this is a good idea, as WaitAny() is overloaded: static bool WaitAny(params UnixSignal[] signals); static bool WaitAny(UnixSignal[] signals, TimeSpan timeout); static bool WaitAny(UnixSignal[] signals, int timeout); Consequently, moving from WaitAny(UnixSignal[]) to one of the other overloads would result in inconsistent usage (adding `new UnixSignal[]{...}', etc.). Alternatively, we could alter the ordering so that things are always consistent, though this would result in a different order from the WaitHandle.WaitAny() methods: static bool WaitAny(params UnixSignal[] signals); static bool WaitAny(TimeSpan timeout, params UnixSignal[] signals); static bool WaitAny(int timeout, params UnixSignal[] signals); Consequently, I'm conflicted: we either keep things as they are, allowing consistency with WaitHandle.WaitAny() and making it easy to change the overload used, or provide params everywhere, and differ from WaitHandle. > > +int > > +Mono_Unix_UnixSignal_WaitAny (void** _signals, int count, int timeout /* > > milliseconds */) > > +{ > > + fd_set read_fds; > > + int mr, r; > > + int max_fd = 0; > > + > > + signal_info** signals = (signal_info**) _signals; > > + > > + mr = pthread_mutex_lock (&signals_mutex); > > + if (mr != 0) { > > + errno = mr; > > + return -1; > > You don't unlock in the return path. if pthread_mutex_lock() returns non-zero, it failed. Am I supposed to call pthread_mutex_unlock() for a failed pthread_mutex_lock() call? The man page doesn't say, vargaz says I shouldn't and should instead assert it's 0, which iirc io-layer also does, and I can't find my Pthreads book, so I'm confused. > An extensive test program that shows this stuff works before being > committed would also help. I only have a small one; I'll need to enhance it. Thanks, - Jon _______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list