<html><head><style type="text/css"><!-- DIV {margin:0px;} --></style></head><body><div style="font-family:times new roman, new york, times, serif;font-size:12pt"><div style="font-family: times new roman,new york,times,serif; font-size: 12pt;">Just curious, which are all the modules need help with? I am interested in contributing. <br>Thanks.<br><br><div style="font-family: times new roman,new york,times,serif; font-size: 12pt;">----- Original Message ----<br>From: John Lam (IRONRUBY) &lt;jflam@microsoft.com&gt;<br>To: IronRuby External Code Reviewers &lt;irbrev@microsoft.com&gt;<br>Cc: "ironruby-core@rubyforge.org" &lt;ironruby-core@rubyforge.org&gt;<br>Sent: Monday, May 5, 2008 12:07:13 PM<br>Subject: [Ironruby-core] Code Review: socket2<br><br>
tfpt review /shelveset:socket2;REDMOND\jflam<br><br>Overall - looks good! We really want to turn on tests for this stuff. Jim's looking at getting the latest rubinius specs running so we can deprecate our current spec snapshot soon - hopefully this week.<br><br>A few minor nits below:<br><br>Seems like you have a couple of redundant Protocols.CastToString calls here - domain is *always* a MutableString based on your ctor ...<br><br>&nbsp; &nbsp; &nbsp; &nbsp; private static Socket CreateSocket(CodeContext/*!*/ context, MutableString/*!*/ domain, MutableString/*!*/ type, object/*Numeric*/ protocol) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; RubyExecutionContext ec = RubyUtils.GetExecutionContext(context);<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; RubyClass rubySocketClass = ec.GetClass(typeof(Ruby.StandardLibrary.RubySocket));<br><br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; AddressFamily addressFamily =
 (AddressFamily)RubyUtils.GetConstant(context, rubySocketClass, SymbolTable.StringToId(Protocols.CastToString(context, domain)), true);<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; ProtocolType protocolType = (ProtocolType)(Protocols.CastToFixnum(context, protocol));<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; SocketType socketType = (SocketType)RubyUtils.GetConstant(context, rubySocketClass, SymbolTable.StringToId(Protocols.CastToString(context, type)), true);<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; return new Socket(addressFamily, socketType, protocolType);<br>&nbsp; &nbsp; &nbsp; &nbsp; }<br><br>Also ... Socket should be Socket/*!*/ since there is only one code path out of this method barring exceptions. I've fixed these minor things in my copy.<br><br>Following this analysis, this also leads to _socket in RubyBasicSocket:23 being a /*!*/ as well.<br><br>BTW, I really like the /*Numeric*/ annotations that you've been putting in your code. This
 could lead us to doing something smarter in the future in the binder like adding a parameter attribute [Numeric] to coerce the binder into doing some smarter things with the conversions.<br><br>Thinking aloud about this method - would it make more sense to have MutableString and int overloads that delegate to helpers rather than dropping everything into a single method like this one here?<br><br>&nbsp; &nbsp; &nbsp; &nbsp; internal static MutableString ConvertToHostString(CodeContext/*!*/ context, object hostname) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (hostname == null) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; return null;<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (hostname is MutableString) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; MutableString strHostname = (MutableString)hostname;<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; //
 Special cases<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (strHostname == "" ) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; strHostname = new MutableString("0.0.0.0");<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; } else if (strHostname == "&lt;broadcast&gt;") {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; strHostname = new MutableString("255.255.255.255");<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; return strHostname;<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; int iHostname;<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (Protocols.IntegerAsFixnum(hostname, out iHostname)) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; // Ruby uses Little Endian whereas .NET uses Big Endian IP values<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
 &nbsp; &nbsp; &nbsp; byte[] bytes = new byte[4];<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; for (int i = 3; i &gt;= 0; --i) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; bytes[i] = (byte)(iHostname &amp; 0xff);<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; iHostname &gt;&gt;= 8;<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; return new MutableString(new System.Net.IPAddress(bytes).ToString());<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; return Protocols.CastToString(context, hostname);<br>&nbsp; &nbsp; &nbsp; &nbsp; }<br><br>Some other points - seems some of the code below could be delegated to the Protocols.CheckSafeLevel() helpers that you added. BTW, we should probably spend some time and make a call about what we're going to do about all of the safe level
 stuff. Arguably this stuff isn't necessary due to CLR safety (especially in Silverlight contexts).<br><br>&nbsp; &nbsp; &nbsp; &nbsp; internal static void CheckSecurity(CodeContext/*!*/ context, object self, string message) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; RubyExecutionContext ec = RubyUtils.GetExecutionContext(context);<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (ec.CurrentSafeLevel &gt;= 4 &amp;&amp; ec.IsObjectTainted(self)) {<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; throw RubyExceptions.CreateSecurityError("Insecure: " + message);<br>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>&nbsp; &nbsp; &nbsp; &nbsp; }<br><br>Thanks,<br>-John<br><br></div><br></div></div></body></html>