[PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

[Anuraag Agrawal]

Hi Chris,

Thanks for the comments!

On Tue, Dec 24, 2019 at 11:57 PM Chris Hegarty <chris.hegarty at oracle.com>
wrote:

>
>
> > On 24 Dec 2019, at 10:12, Aleks Efimov <aleksej.efimov at oracle.com>
> wrote:
> >
> > Hi Anuraag,
> >
> > We need additional approval from openjdk reviewer. After that I will
> sponsor your change.
> >
> > Merry Christmas and a Happy New Year,
> > Aleksei
> >
> > On 18/12/2019 06:08, Anuraag Agrawal wrote:
> >> ...
> >> http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/03/
> >>
>
> I think that this mainly looks good. A few small comments:
>
> 1) If getAdapters returns an error ( ret != ERROR_SUCCESS ), then there
> will be a JNI pending exception, right? so loadDNSConfig0 should not
> unconditionally throw OOME.
>

Thanks for noticing this. Previously, any failure to malloc would return
STS_ERROR and loadDNSConfig0 would throw OOME, while any other type of
error (e.g., getAdaptersInfo returning an error) is ignored. In the current
patch, failure to malloc sets OOME, otherwise a generic Error, and then the
code tries to throw OOME which would probably cause a crash due to throwing
two exceptions in a row without clearing (it's hard to repro the error case
so not entirely sure).

To restore the old behavior, I would have to check the result of
getAdapters and only throw if the exception is OOME, and ignore exceptions
of type Error which are generated here

https://github.com/openjdk/jdk/blob/master/src/java.base/windows/native/libnet/NetworkInterface_winXP.c#L114


Alternatively if leaving the exception of getAdapters as is and just remove
the current JNU_ThrowOutOfMemoryError in loadDNSConfig0, we will throw
Error when a random failure of GetAdapterAddresses happens. This is
probably an incompatible change and not acceptable but just want to confirm.

Also it would be possible to skip getAdapters exceptions that are not OOME
in C or in Java. It would be great to hear what the recommended pattern
would be.


>
> 2)  the output of the append should be “abc,def”, right?
>   53  *  strappend(s1="abc", "def")  => "abc def”
>

Thanks, fixed and will send a new patch with it after finishing 1) based on
advised approach.


>
> 3) I wonder if we should bump MAX_STR_LEN, while here?
>

Seems like a good idea - I don't know what a good value for this is but
noticed another location uses 1K so have raised it to that in my local
copy. Will send a new patch with it after finishing 1).

Thanks a lot,
- Anuraag


>
> -Chris.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20191227/147e626a/attachment.htm>