Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-07-10 Thread Mandy Chung
On 7/10/2013 4:33 PM, Jaroslav Bachorik wrote: >The change looks reasonable. In the class spec for MXBean, suggest to >rename > >interface ThisIsNotMXBean{} > >to something more explicit > >interface NonPublicInterfaceNotMXBean{} Since this was a part of the CCC review which was approve

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-07-10 Thread Jaroslav Bachorik
On 07/09/2013 09:42 PM, Mandy Chung wrote: > On 7/9/13 3:02 AM, Jaroslav Bachorik wrote: >> Please, review the final version of the changes: >> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.07 >> > > The change looks reasonable. In the class spec for MXBean, suggest to > rename > >int

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-07-09 Thread Mandy Chung
On 7/9/13 3:02 AM, Jaroslav Bachorik wrote: Please, review the final version of the changes: http://cr.openjdk.java.net/~jbachorik/8010285/webrev.07 The change looks reasonable. In the class spec for MXBean, suggest to rename interface ThisIsNotMXBean{} to something more explicit i

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-07-09 Thread Jaroslav Bachorik
Please, review the final version of the changes: http://cr.openjdk.java.net/~jbachorik/8010285/webrev.07 It addresses all the concerns raised during the CCC process. I will need at least one official OpenJDK reviewer for the integration. Thanks, -JB-

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-18 Thread Daniel Fuchs
On 6/18/13 1:27 PM, Jaroslav Bachorik wrote: On Tue 18 Jun 2013 12:30:07 PM CEST, Jaroslav Bachorik wrote: On Tue 18 Jun 2013 12:25:35 PM CEST, Daniel Fuchs wrote: Hi Jaroslav, I've added the tests for the proper behaviour when the "com.sun.jmx.mbeans.allowNonPublic" system property is set to

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-18 Thread Jaroslav Bachorik
On Tue 18 Jun 2013 12:30:07 PM CEST, Jaroslav Bachorik wrote: > On Tue 18 Jun 2013 12:25:35 PM CEST, Daniel Fuchs wrote: >> Hi Jaroslav, >> >>> I've added the tests for the proper behaviour when the >>> "com.sun.jmx.mbeans.allowNonPublic" system property is set to true. >>> >>> http://cr.openjdk.ja

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-18 Thread Jaroslav Bachorik
On Tue 18 Jun 2013 12:25:35 PM CEST, Daniel Fuchs wrote: > Hi Jaroslav, > > > I've added the tests for the proper behaviour when the > > "com.sun.jmx.mbeans.allowNonPublic" system property is set to true. > > > > http://cr.openjdk.java.net/~jbachorik/8010285/webrev.05/ > > Thanks for adding the tes

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-18 Thread Daniel Fuchs
Hi Jaroslav, > I've added the tests for the proper behaviour when the > "com.sun.jmx.mbeans.allowNonPublic" system property is set to true. > > http://cr.openjdk.java.net/~jbachorik/8010285/webrev.05/ Thanks for adding the test. I haven't looked at the source again, I trust nothing changed there

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-18 Thread Jaroslav Bachorik
On 06/07/2013 11:07 AM, Daniel Fuchs wrote: >> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.04 > > Hi Jaroslav, > > This looks good to me. > > I assume you've been running both the java.lang & javax.management > unit tests & JCK (java.lang JCK also has some test cases that > indirectly i

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-12 Thread shanliang
Hi, The fix is to address https://jbs.oracle.com/bugs/browse/JDK-8016221 Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8016221/webrev.00/ Instead to use a fixed port to run a JMX connector, we specify the port as 0: JMXServiceURL url = new JMXServiceURL("rmi", null, 0); to allow JMX to se

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-07 Thread Daniel Fuchs
http://cr.openjdk.java.net/~jbachorik/8010285/webrev.04 Hi Jaroslav, This looks good to me. I assume you've been running both the java.lang & javax.management unit tests & JCK (java.lang JCK also has some test cases that indirectly involve JMX introspection). Also, maybe you could add a test

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-07 Thread Jaroslav Bachorik
On Thu 06 Jun 2013 06:06:52 PM CEST, shanliang wrote: > Jaroslav Bachorik wrote: >> On Thu 06 Jun 2013 05:22:31 PM CEST, shanliang wrote: >> >>> Jaroslav, >>> >>> It is now OK for me about the MBean interface searching in the >>> Introspector. >>> >>> Here is my comment on JMX.java: >>> 206 -- 212

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-06 Thread shanliang
Jaroslav Bachorik wrote: On Thu 06 Jun 2013 05:22:31 PM CEST, shanliang wrote: Jaroslav, It is now OK for me about the MBean interface searching in the Introspector. Here is my comment on JMX.java: 206 -- 212 you added a call Introspector.testComplianceMBeanInterface(interfaceClass); I

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-06 Thread Jaroslav Bachorik
On Thu 06 Jun 2013 05:22:31 PM CEST, shanliang wrote: > Jaroslav, > > It is now OK for me about the MBean interface searching in the > Introspector. > > Here is my comment on JMX.java: > 206 -- 212 you added a call > Introspector.testComplianceMBeanInterface(interfaceClass); > > It is better to

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-06 Thread shanliang
Jaroslav, It is now OK for me about the MBean interface searching in the Introspector. Here is my comment on JMX.java: 206 -- 212 you added a call Introspector.testComplianceMBeanInterface(interfaceClass); It is better to move this call to: MBeanServerInvocationHandler.newProxyInstance be

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread Jaroslav Bachorik
On Wed 05 Jun 2013 07:54:10 PM CEST, shanliang wrote: > Daniel Fuchs wrote: >> On 6/5/13 3:55 PM, Jaroslav Bachorik wrote: class A extends B { ...} >class B implements AMBean {...} >>> Yes, I see it now. However, when you check the JMX specification, page >>> 50 onwards, the current imple

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread shanliang
Daniel Fuchs wrote: On 6/5/13 3:55 PM, Jaroslav Bachorik wrote: class A extends B { ...} >class B implements AMBean {...} Yes, I see it now. However, when you check the JMX specification, page 50 onwards, the current implementation does not seem to be correct. "3. If MyClass is an instance of

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread Daniel Fuchs
On 6/5/13 3:55 PM, Jaroslav Bachorik wrote: class A extends B { ...} >class B implements AMBean {...} Yes, I see it now. However, when you check the JMX specification, page 50 onwards, the current implementation does not seem to be correct. "3. If MyClass is an instance of the DynamicMBean inte

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread Jaroslav Bachorik
On Wed 05 Jun 2013 02:52:36 PM CEST, shanliang wrote: > Jaroslav Bachorik wrote: >> On Wed 05 Jun 2013 11:34:05 AM CEST, shanliang wrote: >> >>> Jaroslav Bachorik wrote: >>> On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote: > On Wed 29 May 2013 07:44:34 PM CEST, Dan

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread shanliang
Jaroslav Bachorik wrote: On Wed 05 Jun 2013 11:34:05 AM CEST, shanliang wrote: Jaroslav Bachorik wrote: On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote: On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote: On 5/29/13 7:17 PM, Jaroslav Bachorik wrote:

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread Jaroslav Bachorik
On Wed 05 Jun 2013 11:34:05 AM CEST, shanliang wrote: > Jaroslav Bachorik wrote: >> On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote: >> >>> On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote: >>> On 5/29/13 7:17 PM, Jaroslav Bachorik wrote: > On Wed 29 May 2013 05:33:21 PM CEST,

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-05 Thread shanliang
Jaroslav Bachorik wrote: On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote: On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote: On 5/29/13 7:17 PM, Jaroslav Bachorik wrote: On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote: I would recommend against changing

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-06-03 Thread Jaroslav Bachorik
On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote: > On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote: >> On 5/29/13 7:17 PM, Jaroslav Bachorik wrote: >>> On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote: I would recommend against changing the code to do additional calls to

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-30 Thread Jaroslav Bachorik
On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote: > On 5/29/13 7:17 PM, Jaroslav Bachorik wrote: >> On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote: >>> I would recommend against changing the code to do additional calls to >>> Class.forName during MBean introspection. As I recall w

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Daniel Fuchs
On 5/29/13 7:17 PM, Jaroslav Bachorik wrote: On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote: I would recommend against changing the code to do additional calls to Class.forName during MBean introspection. As I recall we made the opposite change some years ago, both because Class.forNa

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Jaroslav Bachorik
On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote: > I would recommend against changing the code to do additional calls to > Class.forName during MBean introspection. As I recall we made the > opposite change some years ago, both because Class.forName can be slow > (it may call out to a use

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Eamonn McManus
I would recommend against changing the code to do additional calls to Class.forName during MBean introspection. As I recall we made the opposite change some years ago, both because Class.forName can be slow (it may call out to a user ClassLoader) and because it is a potential source of security pro

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Jaroslav Bachorik
Updated webrev - http://cr.openjdk.java.net/~jbachorik/8010285/webrev.01 It adds regtests and takes care of the comments from David and Shanliang. -JB- On 05/28/2013 04:22 PM, Jaroslav Bachorik wrote: > The fix enforces the management interfaces (read MBean and MXBean > interfaces) being public.

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Daniel Fuchs
On 5/29/13 4:44 PM, Jaroslav Bachorik wrote: On Wed 29 May 2013 03:38:59 PM CEST, Daniel Fuchs wrote: On 5/29/13 1:18 PM, shanliang wrote: Jaroslav, Introspector.java - Line 496 - 515 It is good to do check: (Modifier.isPublic(c.getModifiers()) || MBeanAnalyzer.ALLOW_

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Jaroslav Bachorik
On Wed 29 May 2013 03:38:59 PM CEST, Daniel Fuchs wrote: > On 5/29/13 1:18 PM, shanliang wrote: >> Jaroslav, >> >> Introspector.java >> - >> Line 496 - 515 >> It is good to do check: >> (Modifier.isPublic(c.getModifiers()) || >> MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) >> but i

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Daniel Fuchs
On 5/29/13 1:18 PM, shanliang wrote: Jaroslav, Introspector.java - Line 496 - 515 It is good to do check: (Modifier.isPublic(c.getModifiers()) || MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) but it is not necessary if an interface is not equal to clMBeanName. is it possible to

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Jaroslav Bachorik
On Wed 29 May 2013 01:18:50 PM CEST, shanliang wrote: > Jaroslav, > > Introspector.java > - > Line 496 - 515 > It is good to do check: > (Modifier.isPublic(c.getModifiers()) || > MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) > but it is not necessary if an interface is not equal to c

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread shanliang
Jaroslav, Introspector.java - Line 496 - 515 It is good to do check: (Modifier.isPublic(c.getModifiers()) || MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) but it is not necessary if an interface is not equal to clMBeanName. is it possible to simplify the method as: private st

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread Jaroslav Bachorik
On Wed 29 May 2013 10:09:38 AM CEST, David Holmes wrote: > Hi Jaroslav, > > Just wondering why this needs to be public: > > + public static void testComplianceMBeanInterface(Class > interfaceClass) > + throws NotCompliantMBeanException{ > + StandardMBeanIntrospector.getInstance().ge

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-29 Thread David Holmes
Hi Jaroslav, Just wondering why this needs to be public: + public static void testComplianceMBeanInterface(Class interfaceClass) + throws NotCompliantMBeanException{ + StandardMBeanIntrospector.getInstance().getAnalyzer(interfaceClass); + } Same question goes for the exi

Re: jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

2013-05-28 Thread Jaroslav Bachorik
And the webrev would come handy, of course. http://cr.openjdk.java.net/~jbachorik/8010285/webrev.00/ -JB- On 05/28/2013 04:22 PM, Jaroslav Bachorik wrote: > The fix enforces the management interfaces (read MBean and MXBean > interfaces) being public. While this is defined in the specification it