Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpf: simplify adding/removing types to alignchecker #24736

Merged
merged 1 commit into from Apr 4, 2023

Conversation

aspsk
Copy link
Contributor

@aspsk aspsk commented Apr 4, 2023

Types in alignchecker were listed like TYPE _n, e.g., struct tunnel_key _53, so this was simple to add a new type: just add 1 to the last n. However, this is not obvious what to do when removing a type: to re-enumerate the remaining types or to leave holes? Fix this by using the __COUNTER__ macro. Now a new type can be listed as _(TYPE), e.g., _(struct tunnel_key), so this simple both to add and to remove types.

@aspsk aspsk added the release-note/misc This PR makes changes that have no direct user impact. label Apr 4, 2023
@aspsk aspsk requested a review from a team as a code owner April 4, 2023 15:14
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, what a spell! Took 10min to understand three lines lol

@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

Oh wow, what a spell! Took 10min to understand three lines lol

Happy that you were chosen for the review :-)

@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

This PR doesn't require to run the testsuite, as cilium would already fail by this moment if bpf_alignchecker.o is broken.

@aspsk aspsk added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 4, 2023
@pchaigno pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 4, 2023
bpf/bpf_alignchecker.c Outdated Show resolved Hide resolved
Types in alignchecker were listed like 'TYPE _n', e.g., 'struct tunnel_key _53',
so this was simple to add a new type: just add 1 to the last n. However, this
is not obvious what to do when removing a type: to re-enumerate the remaining
types or to leave holes? Fix this by using the __COUNTER__ macro. Now a new
type can be listed as _(TYPE), e.g., '_(struct tunnel_key)', so this simple
both to add and to remove types.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
@aspsk aspsk force-pushed the aspsk/pr/simplify-alignchecker branch from b52cb9e to bea74a9 Compare April 4, 2023 17:32
@aspsk aspsk requested a review from pchaigno April 4, 2023 18:10
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new version was well worth posting a reject review 😄 Thanks! It looks great!

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 4, 2023
@borkmann borkmann merged commit 7ff2a65 into master Apr 4, 2023
43 checks passed
@borkmann borkmann deleted the aspsk/pr/simplify-alignchecker branch April 4, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants