RFR:8190843 can not set/get extendedOptions to ServerSocket

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR:8190843 can not set/get extendedOptions to ServerSocket

Vyom Tewari

Hi All,

Please review the small code change for the below issue.

Webrev     : http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html

BugId        : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which will always be true for ServerSocket as in case of server socket we set "serverSocket" not "socket".

Thanks,

Vyom

Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Chris Hegarty
Vyom,

On 16/11/17 09:03, vyom tewari wrote:

> Hi All,
>
> Please review the small code change for the below issue.
>
> Webrev     :
> http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html
>
> BugId        : https://bugs.openjdk.java.net/browse/JDK-8190843
>
> In this code change, i removed the check(getSocket() == null) which will
> always be true for ServerSocket as in case of server socket we set
> "serverSocket" not "socket".

I think that this change is good. What will happen if SO_FLOW is set
on a ServerSocket?

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Vyom Tewari
Hi Chris,

Thanks for the review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html)
where i modified the code to take care of SO_FLOW for Solaris.

I updated the test code as well.

Thanks,

Vyom


On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:

> Vyom,
>
> On 16/11/17 09:03, vyom tewari wrote:
>> Hi All,
>>
>> Please review the small code change for the below issue.
>>
>> Webrev     :
>> http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html
>>
>> BugId        : https://bugs.openjdk.java.net/browse/JDK-8190843
>>
>> In this code change, i removed the check(getSocket() == null) which
>> will always be true for ServerSocket as in case of server socket we
>> set "serverSocket" not "socket".
>
> I think that this change is good. What will happen if SO_FLOW is set
> on a ServerSocket?
>
> -Chris.

Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Doychin Bondzhev
Hi Vyom,

There is a typo in the comment :

whis is not applicable

Should be "which"


On 23.11.2017 г. 11:20, vyom tewari wrote:

> Hi Chris,
>
> Thanks for the review, please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html)
> where i modified the code to take care of SO_FLOW for Solaris.
>
> I updated the test code as well.
>
> Thanks,
>
> Vyom
>
>
> On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:
>> Vyom,
>>
>> On 16/11/17 09:03, vyom tewari wrote:
>>> Hi All,
>>>
>>> Please review the small code change for the below issue.
>>>
>>> Webrev     :
>>> http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html
>>>
>>> BugId        : https://bugs.openjdk.java.net/browse/JDK-8190843
>>>
>>> In this code change, i removed the check(getSocket() == null) which
>>> will always be true for ServerSocket as in case of server socket we
>>> set "serverSocket" not "socket".
>>
>> I think that this change is good. What will happen if SO_FLOW is set
>> on a ServerSocket?
>>
>> -Chris.
>
>

--
Doychin Bondzhev
dSoft-Bulgaria Ltd.
PowerPro - billing & provisioning solution for Service providers
PowerStor - Warehouse & POS
http://www.dsoft-bg.com/
Mobile: +359888243116

doychin.vcf (280 bytes) Download Attachment
smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Chris Hegarty
In reply to this post by Vyom Tewari

> On 23 Nov 2017, at 09:20, vyom tewari <[hidden email]> wrote:
>
> Hi Chris,
>
> Thanks for the review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) where i modified the code to take care of SO_FLOW for Solaris.

I think this is fine Vyom, thanks.

-Chris.

Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Vyom Tewari
In reply to this post by Doychin Bondzhev
Hi Doychin,

Thanks for review, i will fix it before pushing.

Thanks,

Vyom


On Thursday 23 November 2017 03:01 PM, Doychin Bondzhev wrote:

> Hi Vyom,
>
> There is a typo in the comment :
>
> whis is not applicable
>
> Should be "which"
>
>
> On 23.11.2017 г. 11:20, vyom tewari wrote:
>> Hi Chris,
>>
>> Thanks for the review, please find the latest
>> webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html)
>> where i modified the code to take care of SO_FLOW for Solaris.
>>
>> I updated the test code as well.
>>
>> Thanks,
>>
>> Vyom
>>
>>
>> On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:
>>> Vyom,
>>>
>>> On 16/11/17 09:03, vyom tewari wrote:
>>>> Hi All,
>>>>
>>>> Please review the small code change for the below issue.
>>>>
>>>> Webrev     :
>>>> http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html
>>>>
>>>> BugId        : https://bugs.openjdk.java.net/browse/JDK-8190843
>>>>
>>>> In this code change, i removed the check(getSocket() == null) which
>>>> will always be true for ServerSocket as in case of server socket we
>>>> set "serverSocket" not "socket".
>>>
>>> I think that this change is good. What will happen if SO_FLOW is set
>>> on a ServerSocket?
>>>
>>> -Chris.
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Vyom Tewari
In reply to this post by Chris Hegarty
Hi Chris,

Thanks for review, while my testing i discovered issue in the way we
handle extended  socket options and standard socket options. I fixed it
and updated one test as well.

I removed one redundant "if check" which i think not required. JPRT is
clean with the changed code.

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.2/index.html).

Thanks,

Vyom


On Thursday 23 November 2017 06:04 PM, Chris Hegarty wrote:
>> On 23 Nov 2017, at 09:20, vyom tewari <[hidden email]> wrote:
>>
>> Hi Chris,
>>
>> Thanks for the review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) where i modified the code to take care of SO_FLOW for Solaris.
> I think this is fine Vyom, thanks.
>
> -Chris.
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

roger riggs
In reply to this post by Vyom Tewari
Hi Vyom,

Looks fine

Roger


On 11/16/2017 4:03 AM, vyom tewari wrote:

Hi All,

Please review the small code change for the below issue.

Webrev     : http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html

BugId        : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which will always be true for ServerSocket as in case of server socket we set "serverSocket" not "socket".

Thanks,

Vyom


Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

Chris Hegarty
In reply to this post by Vyom Tewari

> On 1 Dec 2017, at 07:13, vyom tewari <[hidden email]> wrote:
>
> Hi Chris,
>
> Thanks for review, while my testing i discovered issue in the way we handle extended  socket options and standard socket options. I fixed it and updated one test as well.
>
> I removed one redundant "if check" which i think not required. JPRT is clean with the changed code.
>
> Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.2/index.html).

Thanks Vyom, looks good.

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

roger riggs
+1

On 12/2/2017 4:17 AM, Chris Hegarty wrote:

>> On 1 Dec 2017, at 07:13, vyom tewari <[hidden email]> wrote:
>>
>> Hi Chris,
>>
>> Thanks for review, while my testing i discovered issue in the way we handle extended  socket options and standard socket options. I fixed it and updated one test as well.
>>
>> I removed one redundant "if check" which i think not required. JPRT is clean with the changed code.
>>
>> Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.2/index.html).
> Thanks Vyom, looks good.
>
> -Chris.