[meta] Add check for struct/union binary backward compatibility#1795
[meta] Add check for struct/union binary backward compatibility#1795kcudnik merged 7 commits intoopencomputeproject:masterfrom
Conversation
since commit 8e1fb37 (v1.8.0) there is check enum binary backward compatibility so we are relaxing this check below, versions do not have to be equal but both need to be at least 2ebde24 (v1.9.0), this will make sure that enums are always ok, but since that commit some data structures changed and are not binary backwad compatible like next hop group api, acl_capability, structs are backward binary compatible from commit aed34c8, which closest tag version for it is commit 3ff228a (v1.12.0), so min version check should be set to (v1.12.0), but some production branches do not use that high SAI version yet, some use even version v1.7.0 which then it is impossible to provide even enum backward compatibility, check those links: opencomputeproject/SAI#1297 (enums) opencomputeproject/SAI#1795 (structs)
* [configure] Check for minimal SAI version since commit 8e1fb37 (v1.8.0) there is check enum binary backward compatibility so we are relaxing this check below, versions do not have to be equal but both need to be at least 2ebde24 (v1.9.0), this will make sure that enums are always ok, but since that commit some data structures changed and are not binary backwad compatible like next hop group api, acl_capability, structs are backward binary compatible from commit aed34c8, which closest tag version for it is commit 3ff228a (v1.12.0), so min version check should be set to (v1.12.0), but some production branches do not use that high SAI version yet, some use even version v1.7.0 which then it is impossible to provide even enum backward compatibility, check those links: opencomputeproject/SAI#1297 (enums) opencomputeproject/SAI#1795 (structs) * Fix condition
|
TODO mark BEGIN_COMMIT=aed34c8 as v1.12.0, and add README.me with explanation and examples |
| CHECK_STRUCT_SIZE(sai_u32_list_t, 16); | ||
| CHECK_STRUCT_SIZE(sai_u32_range_t, 8); | ||
| CHECK_STRUCT_SIZE(sai_u8_list_t, 16); | ||
| CHECK_STRUCT_SIZE(sai_vlan_list_t, 16); |
There was a problem hiding this comment.
All these structs are part of
https://github.com/opencomputeproject/SAI/blob/6f9d06e9d046c701b273574bcc42e6bc34ed6eba/meta/saimetadatatypes.h#L478C3-L478C24
Imposing this check will not let us define a new structure without impacting the size of sai_attr_value_type_t.
We need to overcome this else we will NEVER be able to add new structures.
There was a problem hiding this comment.
thats the point to make it const and be backward compatible
There was a problem hiding this comment.
you can still add new structures, but less in size of current value size
|
We should add headroom to this union. Some fixed size dummy struct with max
allowed size.
…On Fri, Jun 23, 2023 at 12:13 PM Kamil Cudnik ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In meta/saisanitycheck.c
<#1795 (comment)>
:
> + CHECK_STRUCT_SIZE(sai_segment_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_service_method_table_t, 16);
+ CHECK_STRUCT_SIZE(sai_stat_capability_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_stat_capability_t, 8);
+ CHECK_STRUCT_SIZE(sai_system_port_config_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_system_port_config_t, 24);
+ CHECK_STRUCT_SIZE(sai_timespec_t, 16);
+ CHECK_STRUCT_SIZE(sai_tlv_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_tlv_t, 40);
+ CHECK_STRUCT_SIZE(sai_u16_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_u16_range_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_u16_range_t, 4);
+ CHECK_STRUCT_SIZE(sai_u32_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_u32_range_t, 8);
+ CHECK_STRUCT_SIZE(sai_u8_list_t, 16);
+ CHECK_STRUCT_SIZE(sai_vlan_list_t, 16);
you can still add new structures, but less in size of current value size
—
Reply to this email directly, view it on GitHub
<#1795 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKCSHLMDQ6AKPCI5CBOSW73XMXTFVANCNFSM6AAAAAAYAQNB4A>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
|
Can you please point to the readme with explanation and examples. During the community review, Rita suggested to create a document with examples as to how to handle the case where we need to change the size of an existing struct (for example, when we need add a new field in a key like sai_fdb_entry_t). |
|
the whole point its you CAN'T change the structures, from that commit, size of structures, members sizes and types and order are constant. Please refer to description of this PR, if something is unclear, i will explain. I think if you want to create new entry members, just copy one of existing one, add what you need, add specific objects, etc, bring up PR, and we will be discussing your changes. |
4 participants
This change will force future PRs to be binary backward compatible with structs and unions. This was added to keep all code binary backward compatible if for example, existing code will be using struct array for some sai type, and then only binary sai library will be updated. This change will make sure that no binary break will be possible.
Added restrictions:
Extension headers are ignored in this check.
There maybe some exceptions added in the future for some non essential structures, which would break backward compatibility.