I like the new format better, but the code looks confusing w/o
appropriate comments explaining why prefix is only applied for one type
of arguments.
The old toStringIndented(...) will indent and place whatever obj arg
according to prefix, but the new impl ignores the prefix except for one
type of arg.
This kind of usage fits your new format, but if toStringIndented(...)
were to be used as an utility method for printing out Vector and String
objects, then it'd be a surprise to the callers that prefix is simply
ignored. In a perfect world, they should be in a separate method since
they aren't "indented" using the prefix as the method name implied. But
having enough comments to make it clear is sufficient for this since
it's not used by other classes.
The update webrev looks good.
Valerie
On 08/09/12 20:51, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/7184815/webrev.01/
Add comments before toString and inside toStringIndented.
With the new impl, the responsibility for doing indention when
printing out the String 'obj' is the caller's responsibility.
Well, not really. The indentation is removed in the new impl because
there is no more indentation now, i.e. the String is printed *inline*.
Compare the old output:
key = {
value
}
to new output
key = value
The old impl looks like everything is a collection. In the new impl,
String, Vector and Hashtable have completely different format and are
easily distinguishable.
Thanks
Max
On 08/10/2012 07:18 AM, Valerie (Yu-Ching) Peng wrote:
Max,
Please find my comments in line.
On 08/08/12 19:33, Weijun Wang wrote:
On 08/09/2012 10:10 AM, Valerie (Yu-Ching) Peng wrote:
Max,
The new ordering for reading the config files sounds reasonable.
However, I have questions on scenarios where the various config files
don't exist and the corresponding handling.
The new impl of getJavaFileName()/getNativeFileName() return file
names
even when the file doesn't exist.
This will lead to an IOException when loadConfigFile(String) is
called.
Is this intended?
Yes. But this IOException will be ignored at the end of the private
constructor. This is the also the old behavior. The main purpose is
that even there is no (any kind of) krb5.conf there is still a chance
to read config from DNS etc.
Ok, after re-read the old impl, I agree that the current behavior seems
to match the old one.
Also, the toStringIndented(...) method removed several calls which
append the 'prefix' and made various adjustments.
Do you have sample output using the new impl? Reading through the
code,
some don't look quite right.
There are 2 major changes:
1. A string value does not starts from a newline
2. Vectors are inside square brackets.
Hmm, now I see what your new code is doing. However, it's a bit obscure
and hard to understand as far as I am concerned.
With the new impl, the responsibility for doing indention when printing
out the String 'obj' is the caller's responsibility.
When given a String 'obj', the toStringIndented(...) ignores the given
prefix value, and only append the String 'obj' followed by a '\n'. Same
goes for the Vector 'obj', the prefix is not used at all.
Can you add additional comments to make this clear? I think it'd be good
to include the syntax of your new output, so it's clear that why prefix
is only used for Hashtable 'obj'.
Thanks,
Valerie
For example, for a very typical krb5.conf
[libdefaults]
default_realm = R
[realms]
R = {
kdc = k1
kdc = k2
}
the old toString is
libdefaults = {
default_realm = {
R
}
}
realms = {
R = {
kdc = {
k1
k2
}
}
}
and the new output is
{
libdefaults = {
default_realm = R
}
realms = {
R = {
kdc = [k1,k2,]
}
}
}
Thanks
Max
Thanks,
Valerie
On 08/02/12 06:41, Weijun Wang wrote:
Ping again.
On 07/18/2012 02:29 PM, Weijun Wang wrote:
7184815: [macosx] Need to read Kerberos config in files
Please take a review:
http://cr.openjdk.java.net/~weijun/7184815/webrev.00/
I break the config setting to Java setting and native setting, and
insert the reading of SCDynamicStoreConfig between the two. This
should
preserve the 6u behavior and add a fallback to legacy config files.
No new regression test, because of SCDynamicStoreConfig and system
config files, will ask SQE to create a manual test.
Thanks
Max
On 07/18/2012 08:26 AM, Weijun Wang wrote:
I'm not familiar with how Mac does it, but normally there are two
ways a
Kerberos authentication is performed, through the initial login and
through kinit. The former is integrated into the system (a pam
module?)
and I guess in this case the config is inside
SCDynamicStoreConfig. For
the latter, the Kerberos clients are regarded as standalone tools
and a
/etc/krb5.conf is needed.
Java works in both ways, if there is already a credentials cache it
will
happily use it. On the other hand, it also includes the
Krb5LoginModule
that does all the login itself. Therefore, it should read both
styles of
config on a Mac.
I've filed a new bug, It will appear soon at
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7184815
Thanks
Max
On 07/17/2012 10:35 PM, Mike Swingler wrote:
On Jul 16, 2012, at 8:32 PM, Weijun Wang <weijun.w...@oracle.com>
wrote:
Ping again.
On 07/05/2012 04:34 PM, Weijun Wang wrote:
Hi Scott
On Mac since Lion, sun.security.krb5.Config tries to locate the
config
info in this order:
1. java.security.krb5.conf system property
2. ${jre}/lib/security/krb5.conf
3. SCDynamicStoreConfig
The main difference from other platforms is that it will not try
config
files, say, /Library/Preferences/edu.mit.Kerberos or
/etc/krb5.conf.
On the other hand, even /usr/bin/kinit comes with Lion reads the
config
file (if there is no SCDynamicStoreConfig setting).
Is there a special reason for the current Java behavior? I do
notice
that the Apple 6u33 already does this.
No special reason I can think of, beyond simply swapping the
implementation to read from the SCDynamicStoreConfig. Java SE 6
had
previously had been relying on the system to write out a
/Library/Preferences/edu.mit.Kerberos file, but that went away
with OS
X 10.7, so we didn't see much point in reading the file, since
little
else on the system would be paying attention to it either for the
purposes of SSO.
It seems perfectly reasonable that if there are no
SCDynamicStoreConfig entries, falling back to reading the legacy
config files may be a valid option. I'm actually somewhat
surprised
that they are consulted by kinit.
Regards,
Mike Swingler
Apple Inc.
