[Mono-dev] FW: RealTimeSignal patch

[Jonathan Pryor]

It looks good.  I see only a few issues.

On Tue, 2008-12-16 at 15:54 +0000, tim.jenks at realtimeworlds.com wrote:
> Index: class/Mono.Posix/Mono.Unix.Native/Stdlib.cs
> ===================================================================
> --- class/Mono.Posix/Mono.Unix.Native/Stdlib.cs (revision 121281)
> +++ class/Mono.Posix/Mono.Unix.Native/Stdlib.cs (working copy)
> @@ -504,6 +504,16 @@
>  
>                 public static int SetSignalAction (Signum signal, SignalAction action)
>                 {
> +                       return SetSignalAction(NativeConvert.FromSignum (signal), action);

Space before parenthesis (and elsewhere in the patch).

> Index: class/Mono.Posix/Test/Mono.Unix/UnixSignalTest.cs
> ===================================================================
> --- class/Mono.Posix/Test/Mono.Unix/UnixSignalTest.cs   (revision 121281)
> +++ class/Mono.Posix/Test/Mono.Unix/UnixSignalTest.cs   (working copy)
> @@ -3,11 +3,13 @@
>  //
>  // Authors:
>  //     Jonathan Pryor  <jonpryor at vt.edu>
> +//     Tim Jenks       <tim.jenks at realtimeworlds.com>
>  //
>  // (C) 2008 Jonathan Pryor
>  //
>  
>  using NUnit.Framework;
> +using NUnit.Framework.SyntaxHelpers;
>  using System;
>  using System.Text;
>  using System.Threading;
> @@ -18,31 +20,171 @@
>  
>         [TestFixture]
>         public class UnixSignalTest {
> +
>                 [Test]
> -               public void TestRaise ()
> +               public void TestRealTimeCstor()
>                 {
> -                       Thread t1 = new Thread (delegate () {
> -                                       using (UnixSignal a = new UnixSignal (Signum.SIGINT)) {
> +                       RealTimeSignum rts = new RealTimeSignum(0);
> +                       using (UnixSignal s = new UnixSignal(rts))
> +                       {
> +                               Assert.That(s.IsRealTimeSignal);
> +                               Assert.That(s.RealTimeSignum, Is.EqualTo(rts));
> +                       }
> +               }
> +
> +               [Test]
> +               [ExpectedException(typeof(ArgumentOutOfRangeException))]
> +               public void TestRealTimeOutOfRange()
> +               {
> +                       RealTimeSignum rts = new RealTimeSignum(int.MaxValue);

RealTimeSignum tests should be moved into a class/Mono.Posix/Test/Mono.Unix.Native/RealTimeSignum.cs file.

> +               // helper method to create a thread waiting on a UnixSignal
> +               private Thread WaitSignal(UnixSignal signal, int timeout)'

CreateWaitSignalThread() would be a better name.

> +               {
> +                       Thread t1 = new Thread(delegate() {
>                                                 DateTime start = DateTime.Now;
> -                                               bool r = a.WaitOne (5000, false);
> +                                               bool r = signal.WaitOne (timeout, false);
>                                                 DateTime end = DateTime.Now;
> -                                               Assert.AreEqual (a.Count, 1);
> +                                               Assert.AreEqual (signal.Count, 1);
>                                                 Assert.AreEqual (r, true);
> -                                               if ((end - start) > new TimeSpan (0, 0, 5))
> +                                               if ((end - start) > new TimeSpan (0, 0, timeout/1000))
>                                                         throw new InvalidOperationException ("Signal slept too long");
> -                                       }
> -                       });
> -                       Thread t2 = new Thread (delegate () {
> -                                       Thread.Sleep (1000);
> -                                       Stdlib.raise (Signum.SIGINT);
> -                       });
> -                       t1.Start ();
> -                       t2.Start ();
> -                       t1.Join ();
> -                       t2.Join ();
> +                                       });
> +                       return t1;
>                 }
...
> +               [Test]
> +               public void TestRaiseRTMINSignal()
> +               {
> +                       RealTimeSignum rts = new RealTimeSignum(0);
> +                       Thread t1 = WaitSignal(new UnixSignal(rts), 5000);

You need to ensure that the UnixSignal is disposed, as WaitSignal()
won't (and can't).  (The UnixSignal must be disposed so that it will
unregister itself, otherwise subsequent tests may fail.)  Thus, this
would be better done as:

	RealTimeSignum rts = new RealTimeSignum ();
	using (UnixSignal signal = new UnixSignal (rts)) {
		Thread t1 = CreateWaitSignalThread (signal, 5000);
		Thread t2 = ...;
	}

Alternatively, make CreateWaitSignalThread() higher-level, e.g.

	static void MultiThreadTest (UnixSignal signal, int timeout, ThreadStart b)
	{
		Thread t1 = CreateWaitSignalThread (signal, timeout);
		Thread t2 = new Thread (b);
		t1.Start ();
		t2.Start ();
		t1.Join ();
		t2.Join ();
	}

with a caller of:


	RealTimeSignum rts = new RealTimeSignum ();
	using (UnixSignal signal = new UnixSignal (rts)) {
		MultiThreadTest (signal, 5000, delegate () {
			Thread.Sleep (1000);
			Stdlib.raise (rts);
		});
	}

> Index: class/Mono.Posix/Mono.Unix/UnixSignal.cs
> ===================================================================
> --- class/Mono.Posix/Mono.Unix/UnixSignal.cs    (revision 121281)
> +++ class/Mono.Posix/Mono.Unix/UnixSignal.cs    (working copy)
> @@ -3,6 +3,7 @@
>  //
>  // Authors:
>  //   Jonathan Pryor (jpryor at novell.com)
> +//   Tim Jenks (tim.jenks at realtimeworlds.com)
>  //
>  // (C) 2008 Novell, Inc.
>  //
> @@ -33,28 +34,106 @@
>  using Mono.Unix.Native;
>  
>  namespace Mono.Unix {
> +
> +       public struct RealTimeSignum

This type should be moved into Mono.Unix.Native, so that it's in the
same namespace as Signum.

> +#if NET_2_0
> +                : IEquatable <RealTimeSignum>
> +#endif
> +       {
> +               private int rt_offset;
> +               public static readonly RealTimeSignum MinValue = new RealTimeSignum(0);
> +               public static readonly RealTimeSignum MaxValue = new RealTimeSignum(UnixSignal.GetSIGRTMAX() - UnixSignal.GetSIGRTMIN() - 1);
> +       
> +               public RealTimeSignum(int offset)
> +               {
> +                       if (offset < 0)
> +                               throw new ArgumentOutOfRangeException("Offset cannot be negative");
> +                       if (offset > (UnixSignal.GetSIGRTMAX()-UnixSignal.GetSIGRTMIN()-1))

There's no need to call UnixSignal.GetSIGRTMAX() here, as the MaxValue
field has already computed this.  Just do:

	if (offset > MaxValue.Offset)
		throw new ArgumentOutOfRangeException(...);

> +               public override bool Equals (object obj)
> +               {
> +                       if ((obj == null) || (obj.GetType () != GetType ()))
> +                               return false;

This should delegate to Equals(RealTimeSignum) so that the core logic
isn't duplicated.

> +               public int Offset { 
> +                       get { return rt_offset; }
> +               }

This property should be closer to the constructor, before Equals() and
the overridden operators.

 - Jon