Hi Roger,
Ah, so your alternative to fixing up the regex is to avoid using regexes
entirely! :-)
Seriously, removing the regex is an improvement; it reduces a major source of
fragility.
The update looks fine. I did notice one typo:
82 * Data RMI Regiry bind test.
No need for another webrev for the fix for this one.
Thanks,
s'marks
On 5/31/17 7:09 PM, Roger Riggs wrote:
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
