Hi Roger,
RegistryImpl change looks fine. I have a quibble on the regex used in the test.
162 Matcher m = Pattern
163 .compile(".*maxdepth=(\\d*)")
164 .matcher(Objects.requireNonNullElse(registryFilter, ""));
165 int depthOverride = m.find() ? Integer.parseInt(m.group(1)) :
REGISTRY_MAX_DEPTH;
Since Matcher.find() is called, the pattern can be found anywhere in the input
string, so the leading ".*" is unnecessary. I'd suggest replacing it with "\\b"
to match a word boundary immediately preceding "maxdepth". Also, "\\d*" will
match zero characters, which could cause the subsequent parseInt() to fail.
These issues aren't of immediate concern, as the test works as it stands. They
may make the test a bit harder to maintain, though.
But I think the main concern is that if the regex were modified by future
maintenance, it might not match, which would cause the test to test
REGISTRY_MAX_DEPTH redundantly instead of the depth the filter is attempting to
specify. This problem wouldn't be completely silent, but it'd be easy to miss.
You'd have to inspect the log pretty carefully to see what's going on. So maybe
it would be good to have something like a command-line argument that specifies
the expected value of maxdepth. Yes, this is redundant, but it's a useful
cross-check, I think.
There are several identical lines of code starting at line 184. Maybe these
could be extracted into a common method.
These are all cleanup issues, so it's up to you how much additional work you
want to put into this before it gets into 9.
s'marks
On 5/31/17 7:02 AM, Roger Riggs wrote:
Thanks Daniel for the review and corrected link.
On 5/30/2017 7:04 PM, Daniel Fuchs wrote:
Hi Roger,
On 30/05/17 19:26, Roger Riggs wrote:
Hi Daniel,
Fixed, there is no need for the unbind since the registry is not reused and
it might
cause a spurious success.
(Also corrected the exception error message @ 151 and 153).
Webrev updated in place.
...
I assume you meant
http://cr.openjdk.java.net/~rriggs/webrev-depth-max-8180582/index.html
which is what I reviewed ;-)
Looks good!
-- danel
Thanks, Roger
On 5/30/2017 2:00 PM, Daniel Fuchs wrote:
Hi Roger,
Looks good! Just one nit with the test:
194 registry.unbind(name);
195 Assert.fail("Registry filter should have rejected depth: "
+ depthOverride + 1);
Technically, the test will also pass if bind() succeed but
unbind() fails - for some unforeseen reason.
best regards,
-- daniel
On 25/05/17 16:31, Roger Riggs wrote:
Please review an update to the RMI Registry built-in serial filter
implementation limit on the depth
of a instance that can be bound in the registry. The initial depth limit
was 5 and it was too limiting
for some existing applications. It limit is updated to depth = 20 and
should be more than adequate.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-depth-max-8180582/index.html
Issue:
https://bugs.openjdk.java.net/browse/JDK-8180582
Thanks, Roger
