<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    On 31/12/2015 14:30, Mark Sheppard wrote:<br>
    <blockquote cite="mid:56853BED.2070007@oracle.com" type="cite">
      <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
      Hi<br>
        please oblige and review the current version of the fix  for<br>
      <a moz-do-not-send="true" class="moz-txt-link-freetext"
        href="https://bugs.openjdk.java.net/browse/JDK-8134577">https://bugs.openjdk.java.net/browse/JDK-8134577</a><br>
      <br>
      at<br>
      <a moz-do-not-send="true" class="moz-txt-link-freetext"
        href="http://cr.openjdk.java.net/%7Emsheppar/8134577/webrev.05/">http://cr.openjdk.java.net/~msheppar/8134577/webrev.05/</a><br>
      <br>
      which is  based on feedback from the second review.<br>
    </blockquote>
    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.<br>
    <br>
    InetAddress.nameServices is still a List<NameService> but the
    List will always have one element, should this be changed to
    InetAddress.nameService (singular)?<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    The comment in NameService has a historical reference to the JNDI
    DNS provider, I assume that is not needed.<br>
    <br>
    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:<br>
    String testSrc = System.getProperty("test.src", ".");<br>
    <br>
    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).<br>
    <br>
    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?<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    -Alan<br>
  </body>
</html>