On 19/04/18 21:49, Anil Vishnoi wrote: > It is not a matter of 'it could be supported', anything not defying laws > of Physics can be supported. The question is: does it make sense? can > you show an example when it would be immediately useful? > > That's where i brought the point previously that mdsal should not care > how it can be used or what is the usecase. They should adhere to the > defined types in ietf-types.
This is where it gets tricky and relates to https://git.opendaylight.org/gerrit/#/c/71072/. In order to provide better ergonomics and performance, there are things in MD-SAL, which are providing model-specific hacks in an uniform way. One of these is IetfInetUtil, which is *not* MD-SAL core, but rather model-specific support. You will note this class lives in three places: a generic one, a 2010-09-24 and a 2013-07-15 one. These are classes hand-coded by yours truly. OFP is using these utility methods, i.e. it is binding to a hand-written MD-SAL utility method. The implementation of that class has changed to return Ipv6AddressNoZone, which is a valid subclass of Ipv6Address -- as per mapping we previously discussed. Where things broke is that the returned object no longer equals to Ipv4Address(<same_string>), which is what OFP and others assumed. This is AbstractIetfInetUtil.ipv4AddressFor(). The API contract of that method says: > /** > * Create an Ipv4Address by interpreting input bytes as an IPv4 address. > * > * @param bytes 4-byte array > * @return An Ipv4Address object > * @throws IllegalArgumentException if bytes has length different from 4 > * @throws NullPointerException if bytes is null > */ You assume 'Create an Ipv4Address' to literally mean 'Creates an object whose class is Ipv4Address' -- i.e. excluding subclasses. That is a documentation gap, but I don't know how to improve the text. Patches for clarification are always welcome. I will posit that the text therein can also be interpreted to include Ipv4AddressNoZone -- i.e. a subclass of Ipv4Address, which does not define new API elements and only guarantees that the string contained within is a strict subset of what Ipv4Address can legally hold. The input into this method is an Inet4Address, which has no idea of what a zone is, i.e. the caller has already lost any information about zone it may have had -- which is an important point in a formal proof, if you are into that kind of thing. The only thing that https://git.opendaylight.org/gerrit/#/c/71072/ does is changing the implementation class, i.e. re-wrapping the string contained in the returned Ipv4AddressNoZone through MD-SAL provided guarantee that any Ipv4AddressNoZone string is a valid Ipv4Address string. > > As far as i understood, MDSAL can't implement it in a way I want to use > > (non-breakable), because of language limitation or some other reasons. > > So there are two options > > > > (1) We merge the change and move all the project to these API (given > > that they cannot be used in the way defined in yang model) > > (2) Do not implement support for ipv4-adderss-no-zone (as it's creating > > confusion), and let user use the ipv4-address (which can support no-zone > > and zone ip addresses) > > (3) we change how generated DataObject.equals() works. > > Bye, > Robert > > Unfortunately there is no one else in the community who can mediate > here, it's just your words against my, so i would hold my argument here > and leave it to community. Once a weather reports comes out about this > change and community has no issue with it, i am fine with it. Thank you for your understanding. To drive the argument to conclusion, though, is the analysis why things broke -- which is because Ipv4Address.equals() uses getClass() to determine equality. Ipv4AddressNoZone inherits that -- it's not what Ipv4AddressNoZone type or MD-SAL's treatment of it has done. It is what Ipv4Address as an MD-SAL-generated type does. If we can make Ipv4Address.equals() final, we can define it in way, which would prevent this sort of breakage happening again -- but that is: 1) a restriction on how generated code can be used 2) a relatively high-risk change (is there anybody manually subclassing generated classes?) 3) probably not a 5-line patch implementation The question to the community is: Is that what we want MD-SAL Binding V1 to do Fluorine onwards? Regards, Robert
signature.asc
Description: OpenPGP digital signature
_______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
