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. Output from abipkgdiff with this series applied: ================ changes of 'libopenvswitch-2.so.7.0.0'=============== Functions changes summary: 0 Removed, 14 Changed (84 filtered out), 7 Added functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 7 Added functions: 'function void mcast_snooping_flush_bundle(mcast_snooping*, void*)' {mcast_snooping_flush_bundle@@libopenvswitch_1} 'function ofperr mf_vl_mff_mf_from_nxm_header(uint32_t, const vl_mff_map*, const mf_field**, uint64_t*)' {mf_vl_mff_mf_from_nxm_header@@libopenvswitch_1} 'function ofperr mf_vl_mff_nx_pull_entry(ofpbuf*, const vl_mff_map*, const mf_field**, uint64_t*)' {mf_vl_mff_nx_pull_entry@@libopenvswitch_1} 'function ofperr mf_vl_mff_nx_pull_header(ofpbuf*, const vl_mff_map*, const mf_field**, _Bool*, uint64_t*)' {mf_vl_mff_nx_pull_header@@libopenvswitch_1} 'function void mf_vl_mff_ref(const vl_mff_map*, uint64_t)' {mf_vl_mff_ref@@libopenvswitch_1} 'function void mf_vl_mff_set_tlv_bitmap(const mf_field*, uint64_t*)' {mf_vl_mff_set_tlv_bitmap@@libopenvswitch_1} 'function void mf_vl_mff_unref(const vl_mff_map*, uint64_t)' {mf_vl_mff_unref@@libopenvswitch_1} 14 functions with some indirect sub-type change: [C]'function ofperr bundle_check(const ofpact_bundle*, ofp_port_t, const flow*)' at bundle.c:107:1 has some indirect sub-type changes: return type changed: 1 enumerator insertion: 'ofperr::OFPERR_NXTTMFC_INVALID_TLV_DEL' value '1073741993' 3 enumerator changes: 'ofperr::OFPERR_NXR_NOT_SUPPORTED' from value '1073741993' to '1073741994' 'ofperr::OFPERR_NXR_STALE' from value '1073741994' to '1073741995' 'ofperr::OFPERR_NXST_NOT_CONFIGURED' from value '1073741995' to '1073741996' [C]'function void learn_execute(const ofpact_learn*, const flow*, ofputil_flow_mod*, ofpbuf*)' at learn.c:93:1 has some indirect sub-type changes: parameter 3 of type 'ofputil_flow_mod*' has sub-type changes: in pointed to type 'struct ofputil_flow_mod' at ofp-util.h:287:1: type size changed from 26560 to 26624 bits 1 data member insertion: 'uint64_t ofputil_flow_mod::ofpacts_tlv_bitmap', at offset 26560 (in bits) at ofp-util.h:329:1 [C]'function void mf_vl_mff_map_clear(vl_mff_map*)' at meta-flow.c:2675:1 has some indirect sub-type changes: return type changed: entity changed from 'void' to 'enum ofperr' type size changed from 0 to 32 bits type alignment changed from 0 to 32 bits parameter 2 of type '_Bool' was added [C]'function ofperr nx_pull_match(ofpbuf*, unsigned int, match*, ovs_be64*, ovs_be64*, const tun_table*)' at nx-match.c:612:1 has some indirect sub-type changes: parameter 7 of type 'const vl_mff_map*' was added [C]'function ofperr ofpacts_pull_openflow_actions(ofpbuf*, unsigned int, ofp_version, const vl_mff_map*, ofpbuf*)' at ofp-actions.c:6386:1 has some indirect sub-type changes: parameter 5 of type 'ofpbuf*' changed: in pointed to type 'struct ofpbuf' at stdint.h:55:1: entity changed from 'struct ofpbuf' to 'typedef uint64_t' at stdint.h:55:1 type size changed from 512 to 64 bits parameter 6 of type 'ofpbuf*' was added [C]'function ofperr ofpacts_pull_openflow_instructions(ofpbuf*, unsigned int, ofp_version, const vl_mff_map*, ofpbuf*)' at ofp-actions.c:6939:1 has some indirectsub-type changes: parameter 6 of type 'ofpbuf*' was added [C]'function ofpbuf* ofperr_encode_hello(ofperr, ofp_version, const char*)' atofp-errors.c:238:1 has some indirect sub-type changes: parameter 1 of type 'enum ofperr' has sub-type changes: 1 enumerator insertion: 'ofperr::OFPERR_NXTTMFC_INVALID_TLV_DEL' value '1073741993' 3 enumerator changes: 'ofperr::OFPERR_NXR_NOT_SUPPORTED' from value '1073741993' to '1073741994' 'ofperr::OFPERR_NXR_STALE' from value '1073741994' to '1073741995' 'ofperr::OFPERR_NXST_NOT_CONFIGURED' from value '1073741995' to '1073741996' [C]'function ofperr ofputil_decode_flow_stats_request(ofputil_flow_stats_request*, const ofp_header*, const tun_table*)' at ofp-util.c:2732:1 has some indirect sub-type changes: parameter 4 of type 'const vl_mff_map*' was added [C]'function ofperr ofputil_decode_packet_in(const ofp_header*, _Bool, const tun_table*, ofputil_packet_in*, size_t*, uint32_t*, ofpbuf*)' at ofp-util.c:3482:1 has some indirect sub-type changes: parameter 4 of type 'ofputil_packet_in*' changed: in pointed to type 'struct ofputil_packet_in': entity changed from 'struct ofputil_packet_in' to 'const vl_mff_map' type size changed from 26176 to 448 bits parameter 5 of type 'size_t*' changed: in pointed to type 'typedef size_t' at ofp-util.h:426:1: entity changed from 'typedef size_t' to 'struct ofputil_packet_in' at ofp-util.h:426:1 type size changed from 64 to 26176 bits parameter 7 of type 'ofpbuf*' changed: in pointed to type 'struct ofpbuf' at stdint.h:51:1: entity changed from 'struct ofpbuf' to 'typedef uint32_t' at stdint.h:51:1 type size changed from 512 to 32 bits parameter 8 of type 'ofpbuf*' was added [C]'function ofperr ofputil_decode_packet_in_private(const ofp_header*, _Bool,const tun_table*, ofputil_packet_in_private*, size_t*, uint32_t*)' at ofp-util.c:4062:1 has some indirect sub-type changes: parameter 4 of type 'ofputil_packet_in_private*' changed: in pointed to type 'struct ofputil_packet_in_private': entity changed from 'struct ofputil_packet_in_private' to 'const vl_mff_map' type size changed from 26752 to 448 bits parameter 5 of type 'size_t*' changed: in pointed to type 'typedef size_t' at ofp-util.h:481:1: entity changed from 'typedef size_t' to 'struct ofputil_packet_in_private' at ofp-util.h:481:1 type size changed from 64 to 26752 bits parameter 7 of type 'uint32_t*' was added [C]'function void ofputil_encode_bundle_msgs(const ofputil_bundle_msg*, size_t, ovs_list*, ofputil_protocol)' at ofp-util.c:9550:1 has some indirect sub-type changes: parameter 1 of type 'const ofputil_bundle_msg*' has sub-type changes: in pointed to type 'const ofputil_bundle_msg': in unqualified underlying type 'struct ofputil_bundle_msg' at ofp-util.h:1359:1: type size changed from 26624 to 26688 bits [C]'function ofperr ofputil_pull_ofp11_match(ofpbuf*, const tun_table*, match*, uint16_t*)' at ofp-util.c:280:1 has some indirect sub-type changes: parameter 3 of type 'match*' changed: in pointed to type 'struct match': entity changed from 'struct match' to 'const vl_mff_map' type size changed from 25792 to 448 bits parameter 4 of type 'uint16_t*' changed: in pointed to type 'typedef uint16_t' at match.h:34:1: entity changed from 'typedef uint16_t' to 'struct match' at match.h:34:1 type size changed from 16 to 25792 bits parameter 5 of type 'uint16_t*' was added [C]'function ofperr oxm_decode_match(size_t, const tun_table*, match*)' at nx-match.c:704:1 has some indirect sub-type changes: parameter 2 of type 'const tun_table*' changed: entity changed from 'const tun_table*' to '_Bool' type size changed from 64 to 8 bits parameter 3 of type 'match*' changed: in pointed to type 'struct match': entity changed from 'struct match' to 'const tun_table' type size changed from 25792 to 28992 bits parameter 4 of type 'const vl_mff_map*' was added parameter 5 of type 'match*' was added [C]'function ofperr oxm_pull_match(ofpbuf*, const tun_table*, match*)' at nx-match.c:679:1 has some indirect sub-type changes: parameter 4 of type 'match*' was added ================ end of changes of 'libopenvswitch-2.so.7.0.0'=============== _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
