On 9/6/2018 10:17 AM, Adam Petcher wrote:
In the latest webrev, I changed it so there is a single static
NamedGroupFunctions of each type, and the NamedGroup is passed in as
the first argument to each method that requires it (rather than
being a member of NamedGroupFunctions).
After the re-org, I guess you can put the inner classes in NamedGroup
enum and declare them as private? The getFunctions() method may be
unnecessary then.
I don't know if that works, exactly, due to the fact that I can't
reference static enum members in the body of an enum constructor. How
about this alternative? I can move the NamedGroup enum and all the
NamedGroupFunction classes into a separate class (in a separate file)
called NamedGroups. Then all the NamedGroupFunction classes can be
private in this class, but the NamedGroup enum can still have package
access. I would prefer to leave the getFunctions() method of
NamedGroup (and keep it private) because the functions object may be
missing and the Optional return type of getFunctions() forces me to
deal with this when I call it from within NamedGroup.
I think it should be able to use the "functions" field directly.
Optional ngf = getFunctions()
if (ngf.isEmpty() {
...
}
V.S.
if (functions != null) {
...
}
I did not see the benefits of the getFunctions() method.
Actually, it does work. I just have to move the static members of each
NamedGroupFunctions subclass into its subclass (e.g. make them
singletons). Still, I like my proposed alternative better, because it
allows us to simplify SupportedGroupsExtension. Let me know if you have
a preference.
My concerns are mainly about:
1. the NamedGroupFunctions should be private and should not be used
other than the NamedGroup enum impl.
2. the ffdh/ecdh/xdhFunctions static fields should be private of the
NamedGroup enum as well, and better be lazy instantiated as you are
using Map objects in the NamedGroupFunctions implementation (for
performance).
3. NamedGroupFunctions.namedGroupParams is fine in general, but in this
context, it means the map will always be generated. We used to use a
SupportedGroups to wrap and cache the parameters, and don't care about
the unsupported groups. But in the new re-org, looks like the
unsupported groups may also have a chance to cache/use the parameters.
#3 is a new find when I'm trying to understand your proposal. It would
be nice if you could think about the SupportedGroups impact.
For the question about how it works, there are a few approaches. You can
use singletons as you said or inner enum (See CipherSuite.java).
Xuelei
