RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi,
    as per feedback below webrev has been updated

http://cr.openjdk.java.net/~msheppar/8134577/webrev.06/

change summary:

* List<NameService> nameServices replaced with Nameservice nameService

* references to nameServices removed

* private interface NameService added, with two implementation 
PlatformNameService and HostsFileNameService

* Scanner created with UTF-8  charset

* removed StringTokenizer, replaced with String.split()

* comment handling extended, handling comments on line with mapping entry

* try with resources added to addMappingToHostsFile method in tests

regards
Mark

On 02/01/2016 07:40, Alan Bateman wrote:
> On 31/12/2015 14:30, Mark Sheppard wrote:
>> Hi
>>   please oblige and review the current version of the fix  for
>> https://bugs.openjdk.java.net/browse/JDK-8134577
>>
>> at
>> http://cr.openjdk.java.net/~msheppar/8134577/webrev.05/
>>
>> which is  based on feedback from the second review.
> I looked through the latest webrev and I think this is starting to 
> look good. Having the hosts file be the same format (albeit a subset) 
> as /etc/hosts is a big improvement on previous iterations.
>
> InetAddress.nameServices is still a List<NameService> but the List 
> will always have one element, should this be changed to 
> InetAddress.nameService (singular)?
>
> I think it would be cleaner if NameService were changed to an 
> interface with two implementations, say PlatformNameService and 
> HostsFileNameService. That way you could eliminate the 
> useLocalHostsFileLookup and hostsFileName fields from InetAddress and 
> HostsFileNameService could encapsulate the parsing of the hosts file 
> rather than NameService trying to support both ways.
>
> The property name jdk.net.hosts.file is good but if set to a file that 
> doesn't exist then the current patch falls back to use the platform 
> name service. I assume this is not the original intention.
>
> The Scanner is created without specifying a charset, is this 
> deliberate because the platform /etc/hosts is in platform encoding? 
> For tests then it might be better to use UTF-8 because these hosts 
> file will be used on several different platforms.
>
> Is there any reason to use legacy StringTokenizer in 
> createAddressByteArray? In other areas of the patch then it using 
> Scanner or String.split so it seems inconsistent to see legacy 
> StringTokenizer in use too.
>
> You mentioned in the mail about # supported as a comment when the 
> first character. It doesn't seem to be much effort to just drop 
> everything after the # so that it is more consistent with the platform 
> hosts file.
>
> UHE is thrown in a few places without any exception message. For the 
> hosts file then it would be useful to include some message to make 
> configuration issues easier to diagnose.
>
> The comment in NameService has a historical reference to the JNDI DNS 
> provider, I assume that is not needed.
>
> I also looked through the test changes. Several tests set test.src and 
> not clear that this is needed. I assume that what you really want is:
> String testSrc = System.getProperty("test.src", ".");
>
> addMappingToHostsFile is added to a number of tests. It would be good 
> if this could use try-with-resources to avoid leaving a file open then 
> the write fails (say a test machine with a full file system).
>
> sun/security/x509/URICertStore/ExtensionsWithLDAP.java has been added 
> to the exclude list with JDK-8134577 as the issue number. Is there is 
> a different issue number for this?
>
> Are all of the tests in test/sun/net/InetAddress/nameservice still 
> needed? Some of these tests, as least in the 'dns' directory, are 
> tests for the JNDI DNS provider and maybe they should be deleted.
>
> The update to KDC means that any test using this library will need to 
> run in othervm mode. That may be true already but we should check. 
> Related to this is that setting the system property in the main method 
> might be fragile in that it's possible for code to execute before the 
> main method that triggers the initialization of InetAddress. Something 
> to keep in mind as we might have to re-visit these tests again.
>
> -Alan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160105/d3bae787/attachment.htm>