On Mon, 17 Jun 2024 13:58:11 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - style update >> - whitespace > > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java > line 1314: > >> 1312: return AccessController.doPrivileged(action, acc); >> 1313: } >> 1314: } > > This piece of code is repeated at several places in similar but different > forms - sometime we check for > `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! > SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check > for `acc == null`, sometime for `acc != null` etc.. > I wonder if we could abstract that to some utility method somewhere. Would > that make sense? Maybe that's not possible due to using sometime > `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we > could use `PrivilegedExceptionAction` everywhere at the cost of handling > `PrivilegedActionException`? > If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds > again an equivalent amount of clutter) then maybe we could at least make sure > we do the same checks in the same way (typically `acc == null` vs `acc != > null`)? 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643036701
