> On May 2, 2016, at 7:13 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
>
>
>> One question about:
>> ManagementFactory::getPlatformMXBean(MBeanServerConnection,
>> PlatformLoggingMXBean.class)
>> - what would happen if this method is called from an image with java.logging
>> module present and connect to
>> a VM with no java.logging module?
>> Should the ManagementFactory::getPlatformMXBean spec or
>> PlatformLoggingMXBean spec be clarified?
>
> I don't think there's anything to clarify: it will throw
> an IllegalArgumentException as specified in the spec.
> (it will get an InstanceNotFoundException from the
> remote VM, which will be wrapped and thrown as the
> cause of an IllegalArgumentException)
>
OK. thanks for checking.
>
>>
>> 252 final Map<String, Method> methods =
>> AccessController.doPrivileged(
>> 253 new PrivilegedAction<>() {
>> 254 @Override
>> 255 public Map<String, Method> run() {
>> 256 return initMethodMap(impl);
>> 257 }
>> 258 });
>>
>> I believe this doPrivileged is not necessary and so do the other getMethod
>> calls.
>> Probably you are thinking ahead what if java.management may one day be
>> defined
>> by a child loader of the defining loader of java.logging.
>> Then you can move doPrivileged to initMethodMap.
>
> The doPrivileged around initMethod is necessary because
> the implementation class from which the methods are looked
> up is package protected, so unfortunately Method.setAccessible
> needs to be called. Another possibility would be to lookup
> the method on java.util.logging.LoggingMXBean instead of
> looking them up on the concrete implementation class.
> In that case setAccessible should no longer be needed.
>
> Do you think I should modify the code to do that?
>
I think that’d be cleaner to inspect java.util.logging.LoggingMXBean instead as
Logging class now implements it.
> However your remark made me review the doPrivs and I believe
> the one in getMXBeanImplementation() is not needed, since
> it invokes a public static method on public exported class.
> I removed that.
>
Good.
>
>> 217 // skip
>> - better to throw InternalError since it expects all methods are used?
>
> No. getObjectName() will fall in this bucket - it's not
> defined on LoggingMXBean.
I missed it inspected implementation class rather than LoggingMXBean. Once you
change it to find methods on LoggingMXBean, it should expect all methods can be
found.
>
>> 273 throw new SecurityException(ex);
>> - should not reach here. SecurityException may cause confusion since this
>> will be thrown even without security manager. could simply throw
>> InternalException
>
> OK - Wrapped in UnsupportedOperationException instead.
>
>
>> 296 private PlatformLoggingImpl(LoggingMXBeanSupport support) {
>> - perhaps renaming LoggingMXBeanSupport to LoggingProxy to better represent
>> it.
>
> It's not really a proxy - it only implemements "invoke". Maybe it should
> be called LoggingMXBeanAccess?
>
Sounds good.
>
>>
>> Any impact to permission confiugred in security policy? Should also document
>> that.
>
> The permission are checked against the concrete implementation
> class name "sun.management.ManagementFactoryHelper$PlatformLoggingImpl"
> and we're not changing that - so there should be no incompatibility
> here.
>
Good.
> Thanks a lot for the feedback!
>
> New webrev here:
> http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.07/index.html
Nit: in the class spec for LoggingMXBean.java - wrap type names with {@code…}
such as MBeanServer, PlatformLoggingMXBean.
ManagementFactoryHelper.java
line 333: extra line to be removed?
Mandy
