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: test: Fix the byte order in the IPV4 macro #25114

Merged
merged 1 commit into from Apr 28, 2023

Conversation

gentoo-root
Copy link
Contributor

All the usages suggest that the order of parameters of the IPV4 macro in bpf/tests/pktgen.h is natural: IPV4(192, 168, 0, 1) means 192.168.0.1, not 1.0.168.192. However, the IPV4 macro assembles the bytes in the opposite order, i.e. like 1.0.168.192.

Fix the macro by putting the bytes in the intended order. This change should only have a cosmetic effect when IP addresses are printed in the failed tests, but it might expose some real issues if some tests rely on the IP prefixes.

All the usages suggest that the order of parameters of the IPV4 macro in
bpf/tests/pktgen.h is natural: IPV4(192, 168, 0, 1) means 192.168.0.1,
not 1.0.168.192. However, the IPV4 macro assembles the bytes in the
opposite order, i.e. like 1.0.168.192.

Fix the macro by putting the bytes in the intended order. This change
should only have a cosmetic effect when IP addresses are printed in the
failed tests, but it might expose some real issues if some tests rely on
the IP prefixes.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root gentoo-root added the release-note/misc This PR makes changes that have no direct user impact. label Apr 25, 2023
@gentoo-root gentoo-root requested a review from a team as a code owner April 25, 2023 15:29
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Ah, doing the byte reversing and __bpf_htonl indeed seems superfluous.

@gentoo-root
Copy link
Contributor Author

/test

@pchaigno
Copy link
Member

/test

Isn't this change only covered by the unit tests?

@gentoo-root
Copy link
Contributor Author

/test-1.27-net-next

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 26, 2023
@gentoo-root
Copy link
Contributor Author

Isn't this change only covered by the unit tests?

Yes.

@ti-mo ti-mo merged commit 41d18ff into cilium:main Apr 28, 2023
58 checks passed
@pchaigno
Copy link
Member

Backporting to v1.12 and v1.13 to ease debugging (avoid the red-herring).

@pchaigno pchaigno added needs-backport/1.12 backport/author The backport will be carried out by the author of the PR. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jul 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.10 Jul 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 Jul 28, 2023
@pchaigno pchaigno mentioned this pull request Jul 28, 2023
4 tasks
@pchaigno pchaigno added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jul 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 Jul 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 Jul 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.12.10 Jul 28, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Aug 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.3 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. 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
No open projects
1.13.3
Backport done to v1.13
Development

Successfully merging this pull request may close these issues.

None yet

5 participants