On Mon, 17 Jun 2024 20:27:05 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Hi, >> We almost always check >> !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the >> RMIConnectionImpl contstructor) >> >> This keeps the "modern" case first (except in that constructor, where it is >> only one line for each side of the if...). >> >> This extra checking of >> SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the >> modern and old style cases distinct, based on other comments here. It keeps >> it simple to read I hope, but yes it is a little more verbose than it could >> be. >> >> I'm hoping you'll agree that as these checks will be removed anyway, >> probably with a release, we should delay more extensive cleanup until then. >> (I've also rolled back my noPermissionsACC removal to keep this change away >> from being a general cleanup.) >> >> I had a bunch of additional Exception handling for e.g. >> PrivilegedActionException I think in here, and it wasn't very pretty. But, >> it might look better when there are fewer nested ifs in these methods, and >> when we are properly in the post-SecurityManager world... 8-) >> >> Whether it checks acc != null or acc == null is based on the existing code. >> I think that makes this diff easier to read, but also could be reworked and >> be made more consistent after SM removal. > > Agree with Kevin. To minimize risk, especially since this is to fix a > significant regression and we are in RDP1, we are really trying to preserve > the existing code as much as possible, even though it could be improved. It is also more error prone (which is why I suggested using a single well tested utility method to implement the temporary hackery rather than spreading it at different places in different forms). But I'm OK with the code in this form. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644303322
