RFR 8029994: Support "include" and "includedir" in krb5.conf

[Sean Mullan]

Just a few comments on Config.java:

  479         if (dups.contains(file)) {
  480             throw new IOException("Profile path included more than 
once");
  481         } else {
  482             dups.add(file);
  483         }

This could be compressed into one call as:

if (!dups.add(file)) {
     throw new IOException("Profile path included more than once");
}

506                                 // if dir is abosulte, so is p

typo: absolute

511                 } else if (line.startsWith("include ")) {

why doesn't the file name syntax check on line 505 also apply to include?

570                         public Void run() throws Exception {

This can be declared to throw IOException, then you can change lines 
586-591 to:

throw pe.getException();

--Sean

On 04/10/2014 07:40 AM, Weijun Wang wrote:
> Hi All
>
> Please review the code changes at
>
>     http://cr.openjdk.java.net/~weijun/8029994/webrev.01/
>
> Two major changes made:
>
> 1. The include and includedir directives are supported now. Read
> http://web.mit.edu/kerberos/krb5-current/doc/admin/conf_files/krb5_conf.html
> for a description. The part we support in this RFE is:
>
> -----START-----
> The krb5.conf file can include other files using either of the following
> directives at the beginning of a line:
>
> include FILENAME
> includedir DIRNAME
>
> FILENAME or DIRNAME should be an absolute path. The named file or
> directory must exist and be readable. Including a directory includes all
> files within the directory whose names consist solely of alphanumeric
> characters, dashes, or underscores. Included profile files are
> syntactically independent of their parents, so each included file must
> begin with a section header.
> -----END-----
>
> 2. When the same key appears more than once in krb5.conf, Java used to
> choose the last value, while MIT krb5 chooses the first one. While it's
> debatable whether latecomers should be able to override earlier
> definitions or not, it's more important to have consistent behavior
> across implementations. Therefore we adopt the MIT krb5 way. The
> compatibility risk should be very low since it's very unlikely people
> assigns values to duplicate keys in a single krb5.conf file, which is
> what we support before this enhancement.
>
> One code change that might look strange is in the Config constructor:
>
>           } catch (IOException ioe) {
> -            // I/O error, mostly like krb5.conf missing.
> -            // No problem. We'll use DNS or system property etc.
> +            throw new KrbException(ioe);
>           }
>
> Before this, the only possible IOException thrown is
> FileNotFoundException when krb5.conf is not found, but now there can be
> much more. So I move the FNFE check inside the loadConfigFile() method as
>
> +                Path path = Paths.get(fileName);
> +                if (!Files.exists(path)) {
> +                    // This is OK. There are other ways to get
> +                    // Kerberos 5 settings
> +                    return null;
> +                } else {
> +                    return readConfigFileLines(
> +                            fullp, raw, dupsCheck);
> +                }
>
> Thanks
> Max