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_NONPUBLIC_MBEAN)
but it is not necessary if an interface is not equal to clMBeanName.
is it possible to simplify the method as:
private static <T> Class<? super T> implementsMBean(Class<T> c,
String clName) {
Class<T> ret = null;
try {
Class clMBeanClass = Class.forName(clName + "MBean");
Hi Shanliang,
I 'm not sure whether that would actually simplify anything.
Is attempting to load a class (which BTW would require to pass
the appropriate ClassLoader to Class.forName) faster or simpler
than using the current algorithm?
Well, it seems to simplify the algorithm a bit. Currently, the run
includes walking up the class hierarchy, retrieving all the interfaces
for a class being inspected and the superinterfaces of the interfaces
recursively, to check if any of them complies with the MBean interface
naming. This can turn out to be a quite expensive process.
Additionally, the current implementation seems to go through only 2
levels of interfaces (the interfaces implemented by the inspected class
and their direct super interfaces) and it can fail to resolve an MBean
interface in some corner cases.
On the other hand, when using the Class.forName() it is enough to walk
up the super class hierarchy, try to load a <className>MBean class for
each inspected class and determine whether such interface exists and is
a valid MBean interface. I am not sure how expensive is trying to load
a non-existent class but Class.getInterfaces() is a native method as
well as Class.forName0() generating the same JNI overhead.
-JB-
I would be wary of changing this code without extensive testing.
You will have to read the spec in details and make sure that you
haven't forgotten anything.
For a given implementation class <a.AImpl> there may be
several interfaces that match the MBean interface pattern - and
the spec clearly defines which should have precedence.
For instance if you have:
a.AImpl extends a.A extends b.BImpl extends b.B extends c.C
implements a.AMBean {
}
with c.C implements c.CMBean, b.BMBean { }
then which is the MBean interface for a.AImpl?
I think it's b.BMBean - although I would have to read the spec twice
to be really sure ;-)
-- daniel
But you're right - in isMBeanInterface the first check should
probably be c.getName().equals(clMBeanName).
-- daniel
List list = Arrays.asList(c.getInterfaces());
if (list.contains(clMBeanClass)
&& (Modifier.isPublic(clMBeanClass.getModifiers())
|| MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN)) {
ret = clMBeanClass;
}
} catch (ClassNotFoundException cne) {
// clMBeanClass does not exist?
}
return ret;
}
Is there any special reason to not have a unit test?
Shanliang
Jaroslav Bachorik wrote:
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
was not enforced in any way and it was allowed to create MBeans for
eg.
private MBean interfaces.
The fix adds checks when creating and registering MBeans and throws
javax.management.NotCompliantMBeanException when a user tries to
create
an MBean with non-public management interface.
Since this change can cause problems for users having non-public
management interfaces a system property is introduced that will revert
to the old behaviour when set (com.sun.jmx.mbeans.allowNonPublic).
Thanks,
-JB-
