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

Fix bpf_sock compilation for ipv6-only #30553

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

alexferenets
Copy link

Otherwise, when started with --enable-ipv4=false --enable-ipv6=true I got

level=error msg="Failed to compile bpf_sock.o: exit status 1" compiler-pid=2393 subsys=datapath-loader
level=warning msg="/var/lib/cilium/bpf/bpf_sock.c:1094:10: error: implicit declaration of function '__sock4_health_fwd' [-Werror,-Wimplicit-function-declaration]" subsys=datapath-loader
level=warning msg="                return __sock4_health_fwd(ctx);" subsys=datapath-loader
level=warning msg="                       ^" subsys=datapath-loader
level=warning msg="/var/lib/cilium/bpf/bpf_sock.c:1094:10: note: did you mean '__sock6_health_fwd'?" subsys=datapath-loader
level=warning msg="/var/lib/cilium/bpf/bpf_sock.c:1086:1: note: '__sock6_health_fwd' declared here" subsys=datapath-loader
level=warning msg="__sock6_health_fwd(struct bpf_sock_addr *ctx __maybe_unused)" subsys=datapath-loader
level=warning msg=^ subsys=datapath-loader
level=warning msg="1 error generated." subsys=datapath-loader

@maintainer-s-little-helper
Copy link

Commit 946f9d3 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 31, 2024
@maintainer-s-little-helper
Copy link

Commit 946f9d3 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 31, 2024
@ti-mo ti-mo added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Feb 7, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 7, 2024
@ti-mo ti-mo added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Feb 7, 2024
@ti-mo
Copy link
Contributor

ti-mo commented Feb 7, 2024

@alexferenets Please add a release note to your PR description (you removed it for some reason).

    ```release-note
    Release note here
    ```

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.

We need to fix a point Daniel mentioned, but otherwise, looks good to me.

@ti-mo
Copy link
Contributor

ti-mo commented Feb 23, 2024

@alexferenets Please address the comments, add a commit message (one that includes Signed-off-by as mentioned in the comment above) and remove the merge commit. Just rebasing onto main should take care of that.

@maintainer-s-little-helper
Copy link

Commits 946f9d3, 89d33c5 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@alexferenets alexferenets requested a review from a team as a code owner February 26, 2024 18:51
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 26, 2024
bpf/bpf_sock.c Outdated Show resolved Hide resolved
@brb brb requested review from borkmann and removed request for a team and brb March 1, 2024 09:56
@alexferenets alexferenets force-pushed the fix-ipv6-only branch 2 times, most recently from 70202fb to 87168dc Compare March 1, 2024 12:48
@ldelossa
Copy link
Contributor

ldelossa commented Mar 1, 2024

/test

@ldelossa
Copy link
Contributor

ldelossa commented Mar 1, 2024

@alexferenets - checkpatch is reporting a lint which should be fixed before this is merged: https://github.com/cilium/cilium/actions/runs/8111595771/job/22180953056?pr=30553

@borkmann
Copy link
Member

borkmann commented Mar 4, 2024

@alexferenets - checkpatch is reporting a lint which should be fixed before this is merged: https://github.com/cilium/cilium/actions/runs/8111595771/job/22180953056?pr=30553

Agree, needs proper indent. Otherwise lgtm.

@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 Mar 19, 2024
@borkmann borkmann removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 19, 2024
@borkmann
Copy link
Member

@alexferenets - checkpatch is reporting a lint which should be fixed before this is merged: https://github.com/cilium/cilium/actions/runs/8111595771/job/22180953056?pr=30553

Agree, needs proper indent. Otherwise lgtm.

@alexferenets could you update the indent to a tab? Otherwise its good to go.

@alexferenets alexferenets force-pushed the fix-ipv6-only branch 6 times, most recently from 972c871 to d5ce289 Compare March 19, 2024 12:52
@alexferenets
Copy link
Author

@borkmann can we merge it now?

@julianwiedmann julianwiedmann added the feature/ipv6 Relates to IPv6 protocol support label Apr 1, 2024
Otherwise, when started with --enable-ipv4=false --enable-ipv6=true I got

level=error msg="Failed to compile bpf_sock.o: exit status 1" compiler-pid=2393 subsys=datapath-loader
level=warning msg="/var/lib/cilium/bpf/bpf_sock.c:1094:10: error: implicit declaration of function '__sock4_health_fwd' [-Werror,-Wimplicit-function-declaration]" subsys=datapath-loader
level=warning msg="                return __sock4_health_fwd(ctx);" subsys=datapath-loader
level=warning msg="                       ^" subsys=datapath-loader
level=warning msg="/var/lib/cilium/bpf/bpf_sock.c:1094:10: note: did you mean '__sock6_health_fwd'?" subsys=datapath-loader
level=warning msg="/var/lib/cilium/bpf/bpf_sock.c:1086:1: note: '__sock6_health_fwd' declared here" subsys=datapath-loader
level=warning msg="__sock6_health_fwd(struct bpf_sock_addr *ctx __maybe_unused)" subsys=datapath-loader
level=warning msg=^ subsys=datapath-loader
level=warning msg="1 error generated." subsys=datapath-loader

Signed-off-by: Alex Ferenets <al.ferenets@gmail.com>
@julianwiedmann
Copy link
Member

/test

@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 4, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Apr 4, 2024
Merged via the queue into cilium:main with commit 01d732c Apr 4, 2024
61 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/ipv6 Relates to IPv6 protocol support kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants