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, 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 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 problems.
>>>>>>>>
>>>>>>>>
>>>>>>> Thanks. I was trying to dig some history from mercurial but couldn't.
>>>>>>> Walking through all the related interfaces is equally acceptable - I've
>>>>>>> tried both of the solutions and they test well with the regtests.
>>>>>>>
>>>>>>> I am still puzzled by the current implementation which will fail to
>>>>>>> locate the correct MBean interface in eg.
>>>>>>>
>>>>>>> <<CInterface>> extends <<BInterface>> extends <<ServiceMBean>>
>>>>>>>
>>>>>>> ClassA extends Service implements <<CInterface>>
>>>>>>>
>>>>>>> as the process would stop on <<BInterface>> (checks the superclass of
>>>>>>> the ClassA, checks all the interfaces implemented by the Service class,
>>>>>>> checks all the interfaces extended by <<CInterface>>) which plainly
>>>>>>> does not conform to the MBean interface naming convention and would
>>>>>>> miss the <<ServiceMBean>> interface.
>>>>>>>
>>>>>>>
>>>>>> Hi Jaroslav,
>>>>>>
>>>>>> <<Service>> would have to implement <<ServiceMBean>> either
>>>>>> directly or indirectly.
>>>>>>
>>>>>> So the current implementation is correct.
>>>>>>
>>>>>> If <<ServiceMBean>> is not assignable from <<Service>> then
>>>>>> <<ServiceMBean>> is not an MBean interface for ClassA.
>>>>>>
>>>>>>
>>>>> Actually, when you do
>>>>> ClassA extends Service implements <<BInterface>>
>>>>>
>>>>> the Introspector will return <<ServiceMBean>> as the standard mbean
>>>>> interface for ClassA. I've just tried it on a simple project to make
>>>>> sure I understand the code correctly. The puzzle is which behaviour is
>>>>> correct? Either all the levels of the interface hierarchy should be
>>>>> checked for the [className]MBean interfaces or none, I guess. However,
>>>>> I can not find anything in the spec related to this case.
>>>>>
>>>>>
>>>> I've updated the webrev not to use Class.forName() when inferring the
>>>> MBean interface to use.
>>>>
>>>> However, I've changed the way the interface hierarchy is treated -
>>>> originally, all the interfaces of a certain class and their direct super
>>>> interfaces were checked for the MBean interface candidates (with
>>>> [class.Name]MBean class name) - but only one level of the hierarchy was
>>>> checked. The new implementation will check only the direct interfaces.
>>>>
>>>> I am still not sure whether the interface hierarchy should be searched
>>>> for the appropriately named interfaces or not. The spec does not say
>>>> anything about this and all the jtreg and jck tests are indifferent to
>>>> my changes. But I am sure that if it is supposed to search the interface
>>>> hierarchy it should not stop after the first level has been checked, as
>>>> is done by the current implementation.
>>>>
>>>>
>>> Your modification changes the current JMX behavior, if the spec does
>>> not say about interface searching, I think it is better to keep the
>>> current behavior for the implementation compatibility.
>>>
>>> Here are examples:
>>> 1) A extends B implements AMBean, BMBean
>>> the current JMX will get AMBean for the class A, but BMBean will be
>>> got with your modification.
>>>
>>
>>
> Sorry I was not clear here, I meant:
> class A extends B { ...}
> class B implements BMBean, AMBean
>> No, it will return AMBean. Firstly, the A is interrogated and if there
>> is an AMBean interface in the list of implemented interfaces it is
>> returned.
>>
>>
>>> 2) A extends B implements AMBean
>>> the current JMX will get AMBean for the class A, but which mbean
>>> interface will be found with your modification? I think it will be null.
>>>
>>
>> No, it will return AMBean.
>>
> 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 interface, then
MyClassMBean is
ignored. If MyClassMBean is not a public interface, it is not a JMX
manageable
resource. If the MBean is an instance of neither MyClassMBean nor
DynamicMBean, the inheritance tree of MyClass is examined, looking for the
nearest superclass that implements its own MBean interface.
a. If there is an ancestor called SuperClass that is an instance of
SuperClassMBean, the design patterns are used to derive the attributes and
operations from SuperClassMBean. In this case, the MBean MyClass then
has the same management interface as the MBean SuperClass. If
SuperClassMBean is not a public interface, it is not a JMX manageable
resource.
b. When there is no superclass with its own MBean interface, MyClass is
not a
Standard MBean."
According to the specification the correct MBean interface for
class A extends B { ...}
class B implements BMBean, AMBean
would be BMBean
and for
class A extends B { ...}
class B implements AMBean {...}
is not defined; neither B or A are manageable resources.
As I said, the jtreg and jck test does not seem to mind which
implementation is used, they all pass happily. I would prefer bringing
the implementation in sync with the specification.
-JB-
>> The only thing I've changed is that if you have BMBean extends AMBean
>> and A implements BMBean the AMBean won't be returned. While the current
>> implementation would return AMBean if we add CMBean extends BMBean and
>> then A implements CMBean it will fail to return the AMBean even though
>> it is virtually the same situation. IMO, the implementation should be
>> consistent whether it walks up the interface hierarchy to find the
>> MBean interface or not - and not to choose an arbitrary number of
>> levels that will be checked (currently 2).
>>
>>
>>> 367 if ((Modifier.isPublic(mbeanInterface.getModifiers()) ||
>>> 368 MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) &&
>>> 369 clMBeanName.equals(mbeanInterface.getName())) {
>>> 370 return mbeanInterface;
>>> 371 }
>>> As I indicated in my previous mail, better to write as:
>>> if (clMBeanName.equals(mbeanInterface.getName() &&
>>> (Modifier.isPublic(mbeanInterface.getModifiers()) ||
>>> MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN)) {
>>>
>>> checking name first because it must be cheaper here.
>>>
>>
>> I am not sure - clMBeanName.equals(mbeanInterface.getName()) involves
>> character by character comparison. MBeanAnalyzer.ALLOW_NON_PUBLIC_MBEAN
>> is simple static field access and if mbeanInterface.getModifiers()
>> isn't horribly slow we should be better off with my version. I can
>> profile the code to find out the difference.
>>
> I meant that Call name.equals would filter off all interfaces except
> those which had same name (at most only one in our case?), then we
> needed only to call Modifier.isPublic(mbeanInterface.getModifiers()
> for those same named interfaces.
> Modifier.isPublic(mbeanInterface.getModifiers() would returns true for
> all public interfaces so we had to call name.equals for all public
> interfaces until got same named interface.
>
> Shanliang
>> -JB-
>>
>>
>>> Shanliang
>>>
>>>
>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8010285/webrev.02
>>>>
>>>> -JB-
>>>>
>>>>
>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>>
>>>>>> You can work around that by wrapping an instance of ClassA
>>>>>> in an instance of javax.management.StandardMBean, and by
>>>>>> specifying <<ServiceMBean>>.class as the MBean interface
>>>>>> in the constructor.
>>>>>>
>>>>>> Hope this helps,
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -JB-
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Éamonn
>>>>>>>>
>>>>>>>>
>>>>>>>> 2013/5/29 Jaroslav Bachorik <jaroslav.bacho...@oracle.com
>>>>>>>> <mailto:jaroslav.bacho...@oracle.com>>
>>>>>>>>
>>>>>>>> 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. 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-
>>>>>>>> >
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>
>>
>>
>>
>
