<html><body><p><font size="2">Hi Brent!</font><br><br><font size="2">Please see inline. </font><br><br>> A couple comments on the protocols:<br>> <br>> * `ConnectionDelegate`:<br>> <br>> * Given that the protocol has no methods, only properties, and that `ConnectionType` is concrete, <br>> what behavior is `ConnectionDelegate` abstracting over? Or is it just a placeholder design? <br>> (If it is a placeholder, a comment to that effect might be helpful.)<br><br>It is abstracting over different transport layers cross platform (right now, Linux, but potentially in the future, also windows etc).<br>I agree that the defacto is BSD sockets but this type of abstraction can allow support of other protocols such as STREAMS, etc.<br><br>The question I guess is do we think this support is necessary?<br><br>> * I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both having "delegate" in <br>> their names and (probably) both holding references to each other. The relationship between these two types seems very jumbled.<br><br>TLSServiceDelegate is a delegate to the socket class.<br>ConnectionDelegate is misnamed and can have the delegate removed.<br><br>The relationship is based on: <a href="https://raw.githubusercontent.com/gtaban/blogs/master/TLSServiceArchitecture.png">https://raw.githubusercontent.com/gtaban/blogs/master/TLSServiceArchitecture.png</a><br>(where ConnectionDelegate = Transport Management)<br><br>How do you think we can make improve the relationship? The simpler the better.<br><br><br>> * `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.<br><br><br>yes... but then the acronym wont be capitalized! :-P We really need to think of a good name here :-) Any suggestions?<br><br>> * The `-Type` suffix is reserved for generic parameters by the API Guidelines, so `ConnectionType` is misnamed. <br>> I would suggest `Endpoint` or `ConnectionEndpoint`.<br><br>Good point.<br><br>> * `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?<br><br>I think all connections must have an endpoint so I don't think it makes sense to have it optional. You're right that the unknown case <br>is probbably not useful and we can probably remove it.<br><br>> * `TLSServiceDelegate`:<br><br>> * The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.<br><br>Sorry, why are they inappropriate? I was trying to make it readable.<br><br>> * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and <br>> `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters. <br>> This `willSend` variant should also be unlabeled.<br><br>Makes sense but tbh I was intending to remove those APIs and only support Data. The data type that these methods <br>support depend on the what the socket class will support, which as far as I understand, have not been defined yet.<br><br>Do you think it still makes sense for us to include the Unsafe[Mutable]RawBufferPointer versions of the APIs?<br><br><br>> * "Zero indicates TLS shutdown, less than zero indicates error." Why? Shouldn't we be throwing errors, and either <br>> throwing or returning `nil` for shutdowns? I believe the implementation actually showed some examples of negative <br>> returns indicating errors, which seems to indicate it wasn't accidentally left in.<br><br>@Bill Abt can hopefully comment more on this.<br><br><br>> * I continue to be concerned by the `didCreateClient()` and `didCreateServer()` calls. In the example implementation, <br>> they simply call into methods which, to my mind, ought to be part of initialization. Large swaths of `TLSService` properties <br>> are mutable seemingly entirely to accommodate this half-initialized state. I think splitting the static, shareable configuration <br>> from the dynamic, per-connection SSL delegate (by replacing these two calls with `makeClientDelegate()` and <br>> `makeServerDelegate()` calls on a separate protocol) would substantially clean up this design.<br><br>Interesting, can you give a bit more detail on this?<br>I think potentially what you are suggesting might mean that the socket library needs to be split as well to client and server components. Is that correct?<br><br><br>> * `TLSError`:<br><br>> * I have no idea why `success` should be an error case. `success` is the lack of an error, not an error.<br><br>> * What is the purpose of the `fail` case's error code and string? They're impossible to interpret without some kind of <br>> domain. And at that point...well, you've basically just reinvented `NSError`.<br><br>> * In general, this type seems to fail to actually codify the errors a TLSService could encounter, which limits its usefulness.<br><br>I'll let @Bill Abt reply to this one.<br><br>cheers,<br>Gelareh<br><br><br><img width="16" height="16" src="cid:1__=8FBB0BDDDFCC97958f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for Brent Royal-Gordon ---06/29/2017 07:00:31 AM---> On Jun 28, 2017, at 11:30 AM, Gelareh Taban via swif"><font size="2" color="#424282">Brent Royal-Gordon ---06/29/2017 07:00:31 AM---> On Jun 28, 2017, at 11:30 AM, Gelareh Taban via swift-server-dev <swift-server-dev@swift.org> wrot</font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">Brent Royal-Gordon <brent@architechies.com></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">Gelareh Taban <gtaban@us.ibm.com></font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">swift-server-dev@swift.org</font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">06/29/2017 07:00 AM</font><br><font size="2" color="#5F5F5F">Subject: </font><font size="2">Re: [swift-server-dev] TLSService mini-release</font><br><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br>
<ul><ul>On Jun 28, 2017, at 11:30 AM, Gelareh Taban via swift-server-dev <<a href="mailto:swift-server-dev@swift.org"><u><font color="#0000FF">swift-server-dev@swift.org</font></u></a>> wrote:<br>
<p><font size="2">Hi all,</font><br><font size="2"><br>A quick update on TLS library update.</font><br><font size="2"><br>I have an implementation of the protocol proposed earlier: </font><a href="https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20170320/000298.html"><u><font size="2" color="#0000FF">https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20170320/000298.html</font></u></a></ul></ul>This is good work!
<ul><ul><font size="2">For now, I would like to throw this code out for public review. Please review below and let's look at what needs to get done.</font><br><font size="2"><br>The protocols are defined in:</font><u><font size="2" color="#0000FF"><br></font></u><a href="https://github.com/gtaban/security"><u><font size="2" color="#0000FF">https://github.com/gtaban/security</font></u></a></ul></ul>A couple comments on the protocols:<br><br>* `ConnectionDelegate`:<br><br>* Given that the protocol has no methods, only properties, and that `ConnectionType` is concrete, what behavior is `ConnectionDelegate` abstracting over? Or is it just a placeholder design? (If it is a placeholder, a comment to that effect might be helpful.)<br>* I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both having "delegate" in their names and (probably) both holding references to each other. The relationship between these two types seems very jumbled.<br>* `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.<br>* The `-Type` suffix is reserved for generic parameters by the API Guidelines, so `ConnectionType` is misnamed. I would suggest `Endpoint` or `ConnectionEndpoint`.<br><br>* `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?<br><br>* `TLSServiceDelegate`:<br><br>* The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.<br><br>* The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters. This `willSend` variant should also be unlabeled.<br><br>* "Zero indicates TLS shutdown, less than zero indicates error." Why? Shouldn't we be throwing errors, and either throwing or returning `nil` for shutdowns? I believe the implementation actually showed some examples of negative returns indicating errors, which seems to indicate it wasn't accidentally left in.<br><br>* I continue to be concerned by the `didCreateClient()` and `didCreateServer()` calls. In the example implementation, they simply call into methods which, to my mind, ought to be part of initialization. Large swaths of `TLSService` properties are mutable seemingly entirely to accommodate this half-initialized state. I think splitting the static, shareable configuration from the dynamic, per-connection SSL delegate (by replacing these two calls with `makeClientDelegate()` and `makeServerDelegate()` calls on a separate protocol) would substantially clean up this design.<br><br>* `TLSError`:<br><br>* I have no idea why `success` should be an error case. `success` is the lack of an error, not an error.<br><br>* What is the purpose of the `fail` case's error code and string? They're impossible to interpret without some kind of domain. And at that point...well, you've basically just reinvented `NSError`.<br><br>* In general, this type seems to fail to actually codify the errors a TLSService could encounter, which limits its usefulness.<br><br><font size="2">-- </font><br><font size="2">Brent Royal-Gordon</font><br><font size="2">Architechies</font><br><br><br><BR>
</body></html>