[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-6515161 If remote removeNotificationListener gets SecurityException, client no longer g
From:       Daniel Fuchs <daniel.fuchs () oracle ! com>
Date:       2017-05-09 8:09:14
Message-ID: 96f866ae-ff21-dfdd-4068-06731b2feb3d () oracle ! com
[Download RAW message or body]

+1

Thanks Ujwal!

-- daniel

On 09/05/17 06:38, Ujwal Vangapally wrote:
> Hi Harsha, Daniel,
> 
> updated method names as getListenerIds/getListenerId
> 
> webrev : 
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.02/
> 
> 
> On 5/8/2017 9:57 PM, Daniel Fuchs wrote:
> > Hi Harsha,
> > 
> > Well, I think both methods should have the same name
> > and I don't see any strong reason for changing the old
> > name - since there is no such thing as a 'ClientListener'
> > (there are only NotificationListeners).
> > 
> > best regards,
> > 
> > -- daniel
> > 
> > On 08/05/2017 17:13, Harsha Wardhana B wrote:
> > > Hi Daniel,
> > > 
> > > The term 'listenerid' is used in conjunction with method name/object
> > > field which adds context about the term 'listenerid'. Having a
> > > standalone method name as getClientListenerId is less ambiguous compared
> > > to method name : getListenerId.
> > > 
> > > I don't really have a strong opinion on this and am okay with either 
> > > names.
> > > 
> > > -Harsha
> > > 
> > > 
> > > On Monday 08 May 2017 03:16 PM, Daniel Fuchs wrote:
> > > > On 08/05/2017 09:45, Ujwal Vangapally wrote:
> > > > > Thanks for the Review Daniel, Harsha
> > > > > 
> > > > > Please find the new webrev incorporating the review comments
> > > > > 
> > > > > webrev :
> > > > > http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.01/
> > > > 
> > > > Looks good. In retrospect I wonder if renaming
> > > > getListenerIds/getListenerId into
> > > > getClientListenerIds/getClientListenerId
> > > > was such a good idea given that 'listenerId' is an established
> > > > terminology in the specification [1] [2] (and 'listenerId' is used all
> > > > over the place in ClientNotifForwarder anyway).
> > > > 
> > > > Harsha, what do you think?
> > > > 
> > > > best regards,
> > > > 
> > > > -- daniel
> > > > 
> > > > [1] See @return in
> > > > http://download.java.net/java/jdk9/docs/api/javax/management/remote/rmi/RMICon \
> > > > nection.html#addNotificationListeners-javax.management.ObjectName:A-java.rmi.MarshalledObject:A-javax.security.auth.Subject:A- \
> > > >  
> > > > 
> > > > [2] see @param listenerIds in
> > > > http://download.java.net/java/jdk9/docs/api/javax/management/remote/rmi/RMICon \
> > > > nection.html#removeNotificationListeners-javax.management.ObjectName-java.lang.Integer:A-javax.security.auth.Subject- \
> > > >  
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Ujwal
> > > > > 
> > > > > 
> > > > > On 5/8/2017 12:03 PM, Daniel Fuchs wrote:
> > > > > > Hi Ujwal,
> > > > > > 
> > > > > > For consistency I think the new getListenerIds method should:
> > > > > > 
> > > > > > a) either return an array of Integer, even if it contains only 1
> > > > > > Integer:
> > > > > > 
> > > > > > 1. The name of the method implies that an array is returned
> > > > > > 2. You will need the array when you call
> > > > > > connection.removeNotificationListeners anyway.
> > > > > > 
> > > > > > or b) the other possibility is to remove the 's' at the end of the
> > > > > > method.
> > > > > > 
> > > > > > Both would be acceptable.
> > > > > > 
> > > > > > best regards,
> > > > > > 
> > > > > > -- daniel
> > > > > > 
> > > > > > On 04/05/17 07:59, Ujwal Vangapally wrote:
> > > > > > > corrected webrev link :
> > > > > > > http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/ 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 5/4/2017 12:14 PM, Ujwal Vangapally wrote:
> > > > > > > > 
> > > > > > > > Kindly review the changes made for below bug
> > > > > > > > 
> > > > > > > > Problem description and solution are explained in comments section
> > > > > > > > 
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-6515161
> > > > > > > > 
> > > > > > > > diff for*ClientNotifForwarder.java *might be a bit confusing as it
> > > > > > > > shows the method name
> > > > > > > > 
> > > > > > > > removeNotificationListener is modified to getListenerIds and new
> > > > > > > > method removeNotificationListener is introduced.
> > > > > > > > 
> > > > > > > > Actual change is new method getListenerIds is introduced and it is
> > > > > > > > called in removeNotificationListener method without
> > > > > > > > 
> > > > > > > > affecting the functionality of removeNotificationListener.
> > > > > > > > 
> > > > > > > > webrev :
> > > > > > > > cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > 
> > > > > > > > Ujwal,
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]