RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

[Ivan Gerasimov]

Hi Chris!

Thank you for reviewing the patch set!

On 3/9/18 3:50 AM, Chris Hegarty wrote:
> Ivan,
>
> Thanks for breaking up the changes, it makes it easier to review.
>
> I disagree with changing DualStackPlainSocketImp to conform with
> TwoStacks (and unix/PlainSocketImpl). I would prefer to move the
> latter to the former. Specifically, around the use of fdAccess.
>
Do you think unix/PlainSocketImpl should also be refactored that way at 
some point later?

It looks tempting to make Windows and Unix implementation similar, so 
that any further modifications should they need to be done to both will 
be easier to apply.
That was the primary reason why I choose TwoStacks as the basis and 
aligned DualStack with it.

It can surely be done the opposite way, though I want to make sure it is 
the preferable way to go.

With kind regards,
Ivan


> -Chris.
>
>
> On 06/03/18 20:31, Ivan Gerasimov wrote:
>> In order to make is easier to review the fix, I made the webrev.ksh 
>> to generate a series of incremental webrevs from the mq patch stack.
>>
>> Here's the list of the incremental changes with a brief comments:
>>
>> WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/00/000/webrev/
>>      Only changing the order of methods declaration
>>
>> WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/00/001/webrev/
>>      Renaming initIDs to initProto
>>
>> WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/00/002/webrev/
>>      Changing socketBind
>>
>> WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/00/003/webrev/
>>      Changing socketCreate
>>
>> WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/00/004/webrev/
>>      Changing socketListen
>>
>> WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/00/005/webrev/
>>      Changin socketAvailable
>>
>> WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/00/006/webrev/
>>      Changing socketClose0
>>
>> WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/00/007/webrev/
>>      Changing socketShutdown
>>
>> WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/00/008/webrev/
>>      Changing socketSendUrgentData
>>
>> WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/00/009/webrev/
>>      Changing socketAccept
>>
>> WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/00/010/webrev/
>>      Changing socketConnect
>>
>> WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/00/011/webrev/
>>     Minor editing, comments, moving code
>>
>> WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/00/012/webrev/
>>      Changing socketSetOption
>>
>> WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/00/013/webrev/
>>      Changing socketGetOption
>>
>> WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/00/014/webrev/
>>      Moving a few methods one more time
>>
>>
>> Accumulative webrev with all the changes above is available here:
>> http://cr.openjdk.java.net/~igerasim/8198358/01/webrev/
>>
>>
>> Thanks in advance!
>>
>> Ivan
>>
>> On 3/1/18 8:43 PM, Ivan Gerasimov wrote:
>>> Hello!
>>>
>>> I'd like to do the next step toward removing the TwoStacks socket 
>>> implementation on Windows.
>>>
>>> It would be aligning the two implementations (DualStack and 
>>> TwoStacks), so they can be easier merged together during the next step.
>>>
>>> There are three PlainSocketImpl implementations in JDK:
>>> java.base/windows/classes/java/net/DualStackPlainSocketImpl.java
>>> java.base/windows/classes/java/net/TwoStacksPlainSocketImpl.java
>>> java.base/unix/classes/java/net/PlainSocketImpl.java
>>>
>>> While two later have very similar organization (in particular, set 
>>> of native methods), the former is organized slightly differently.
>>> In order to merge the two Windows implementation together, they 
>>> first need to be organized in a similar way.
>>> For consistency, DualStack implementation will be reorganized to be 
>>> aligned with TwoStacks and unix/PlainSocketImpl.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8198358
>>> Webrev: http://cr.openjdk.java.net/~igerasim/8198358/00/webrev/
>>>
>>> The change looks somewhat messy, but in fact it was a series of 
>>> incremental changes, which I still keep in the mercurial 'mq'.
>>>
>>> (I wish the webrev could be made incremental based on the mq 
>>> patches, to make it easier to review.)
>>>
>>> The patched JDK builds fine and all the regression tests pass Okay.
>>>
>>>
>>> Thanks in advance!
>>
>

-- 
With kind regards,
Ivan Gerasimov