Hi Stuart,
Good suggestions on the regex, but it will be simpler, as you suggest,
to use a separate
command line property.
The webrev is updated in place.
http://cr.openjdk.java.net/~rriggs/webrev-depth-max-8180582/index.html
Thanks, Roger
On 5/31/17 7:09 PM, Stuart Marks wrote:
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
