Hi Chris, Thanks for the comments!
On Tue, Dec 24, 2019 at 11:57 PM Chris Hegarty <chris.hega...@oracle.com> wrote: > > > > On 24 Dec 2019, at 10:12, Aleks Efimov <aleksej.efi...@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.
