On Wed, Apr 26, 2017 at 04:18:47PM -0700, Joe Stringer wrote: > On 21 April 2017 at 09:11, Ben Pfaff <[email protected]> wrote: > > On Thu, Apr 20, 2017 at 06:58:18PM -0700, Joe Stringer wrote: > >> On 17 March 2017 at 14:30, Joe Stringer <[email protected]> wrote: > >> > On 15 March 2017 at 16:01, Joe Stringer <[email protected]> wrote: > >> >> Commit 04f48a68c428 ("ofp-actions: Fix variable length meta-flow > >> >> OXMs."), on > >> >> branch-2.7 as 9554b03d6ab7, attempted to address incorrect encode and > >> >> decode of > >> >> variable length metaflow fields where the OXM/NXM encoding of the > >> >> variable > >> >> length fields would incorrectly serialize the length. The patch > >> >> addresses this > >> >> by introducing a new per-bridge structure that adds additional metaflow > >> >> fields > >> >> for the variable-length fields on demand when the TLVs are configured > >> >> by a > >> >> controller. > >> >> > >> >> Unfortunately, in the original patch there was nothing ensuring that > >> >> flows > >> >> referring to variable length fields would retain valid field references > >> >> when > >> >> controllers reconfigure the TLVs. In practice, this could lead to a > >> >> crash of > >> >> ovs-vswitchd by configuring a TLV field, adding a flow which refers to > >> >> it, > >> >> removing the TLV field, then running some traffic that hit the > >> >> configured flow. > >> >> > >> >> This series looks to remedy the situation by reference counting the > >> >> variable > >> >> length fields and preventing a controller from reconfiguring TLV fields > >> >> when > >> >> there are active flows whose match or actions refer to the field. > >> >> > >> >> This series was applied to master, but given the size of the change and > >> >> the > >> >> minor changes necessary to apply to branch-2.7, I would feel more > >> >> confident in > >> >> backporting it if there was an extra round of review to ensure that > >> >> nothing was > >> >> missed when this series was first applied to master. > >> > > >> > One further concern I have with this series is that while it allows us > >> > to fix bugs in OVS 2.7, it would change some files in > >> > include/openvswitch/, which I believe indirectly implies that it could > >> > break the libopenvswitch ABI, which we try not to do within a release > >> > series: > >> > > >> > http://docs.openvswitch.org/en/latest/internals/contributing/libopenvswitch-abi/ > >> > >> Reporting back, using abipkgdiff from > >> libabigail[https://sourceware.org/libabigail], I was able to identify > >> the following ABI breakages from v2.7.0 to branch-2.7 with these > >> patches applied, details below. > >> > >> A bunch of these are libraries only exported through headers in lib/, > >> which I believe is not considered 'stable' ABI. > >> > >> However, there are several that are exported in include/openvswitch: > >> > >> ofpacts_pull_openflow_instructions > >> ofperr_encode_hello > >> ofputil_decode_flow_stats_request > >> ofputil_decode_packet_in > >> ofputil_decode_packet_in_private > >> ofputil_encode_bundle_msgs > >> ofputil_pull_ofp11_match > >> > >> Now, the shared library is currently something like > >> 'libopenvswitch-2.so.7.0.0' (or, when we prepare 2.7.1, > >> 'libopenvswitch-2.so.7.0.1' unless other changes are made). The > >> libtool ABI numbering does not appear to have any influence on the > >> naming of the shared library. It is (1,0,0) for current, revision and > >> age. I'm suspecting that the right answer to this is to bump current > >> and age as per > >> [https://lists.gnu.org/archive/html/libtool/2009-08/msg00034.html]. > >> When I do this, the ABI appears completely different according to > >> abipkgdiff due to the different 'current' number. > >> > >> I'm not sure if we are supposed to make the ABI versioning number > >> consistent with our shared library naming, or if it is reasonable for > >> each OVS release series (eg, 2.7.x) to have independent libtool > >> versioning numbers. > > > > Thank you for looking into this. > > > > It sounds like for 2.7.1 we should change the library name from > > libopenvswitch-2 to libopenvswitch-2.7.1. For the long term, it sounds > > like maybe we need to include an extra component of the version in the > > library name, so that we'd end up with libopenvswitch-X.Y.so.1.0.Z by > > default. > > > > What are your thoughts? > > Thanks for the thoughts Ben, I took this suggestion and proposed it here: > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331503.html > > I'm not sure that I see why 2.7.1 release would require the full > "2.7.1" number to be included in the name. If we rename it, I think > that it should become effectively a different library so we could > reset the libtool "current" number safely.
I think you're right, thanks. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
