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.

Sign up for GitHub

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

Jump to bottom

bpf_host: declare variables in the beginning of the block #15560

Merged
merged 1 commit into from
Apr 8, 2021
Merged

bpf_host: declare variables in the beginning of the block #15560

merged 1 commit into from
Apr 8, 2021

Conversation

chkhalt
Copy link

@chkhalt chkhalt commented Apr 6, 2021 •
edited

Declaring the variables int trace = TRACE_FROM_HOST; and bool from_proxy; at the beginning of the block allows us to define HOST_REDIRECT_TO_INGRESS without mixing declaration and code.

Updates: #15559

Signed-off-by: Joao Victorino joao@accuknox.com

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 6, 2021
@chkhalt chkhalt marked this pull request as ready for review April 6, 2021 04:14
@chkhalt chkhalt requested review from a team and kkourt April 6, 2021 04:14
kkourt
kkourt reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thank you for the fix. Patch looks good to me, can you please rebase to the current master?

@kkourt kkourt added the release-note/misc This PR makes changes that have no direct user impact. label Apr 6, 2021
@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 Apr 6, 2021
borkmann
borkmann reviewed Apr 6, 2021
View reviewed changes
bpf/bpf_host.c Outdated Show resolved Hide resolved
@chkhalt chkhalt requested review from a team as code owners April 6, 2021 10:51
@chkhalt chkhalt requested review from a team and aditighag April 6, 2021 10:51
@chkhalt chkhalt marked this pull request as draft April 6, 2021 12:16
@chkhalt chkhalt marked this pull request as ready for review April 6, 2021 14:23
@pchaigno
Copy link
Member

pchaigno commented Apr 6, 2021

@johngv2 In #15559, you also mentioned we need to update the bpf/Makefile, which is correct. Do you plan to do that in a different pull request? If that's the case, could you change Fixes: #15559 to Updates: #15559 so GitHub doesn't close the issue when we merge this?

@aditighag aditighag removed their assignment Apr 6, 2021
@chkhalt
Copy link
Author

chkhalt commented Apr 6, 2021 •
edited

For sure! In the PR for #15095 can I refer both issues... like: Fixes: #15095, #15559?

@pchaigno Done! ;)

@pchaigno
Copy link
Member

pchaigno commented Apr 6, 2021

For sure! In the PR for #15095 can I refer both issues... like: Fixes: #15095, #15559?

I think you'll need to have two Fixes on two different lines, but otherwise, yes.

@pchaigno pchaigno added kind/cleanup This includes no functional changes. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Apr 7, 2021
bpf_host: declare variables in the beginning of the block
1cf1432 
That way we can define HOST_REDIRECT_TO_INGRESS without mixing declaration and code.

Updates: #15559

Signed-off-by: Joao Victorino <joao@accuknox.com>
@aanm
Copy link
Member

aanm commented Apr 7, 2021

test-me-please

@pchaigno
Copy link
Member

pchaigno commented Apr 8, 2021 •
edited

k8s-1.21-kernel-4.9 failed with known flake #14959.

All Runtime tests failed with #15604.

test-runtime

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 8, 2021
@pchaigno pchaigno merged commit e83890b into cilium:master Apr 8, 2021
1.10.0 automation moved this from In progress to Done Apr 8, 2021
aanm pushed a commit that referenced this pull request Apr 27, 2021
@aanm
lib/proxy.h: set variable as maybe unused to avoid compilation error
652ccd2 
This allow us to have HOST_REDIRECT_TO_INGRESS defined without a compilation error.
As the code in #15560 is already merged, we define the macro in the Makefile.

Fixes: #15095
Fixes: #15559

Signed-off-by: Joao Victorino <joao@accuknox.com>
This was referenced Apr 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkourt kkourt kkourt left review comments

@borkmann borkmann borkmann approved these changes

@pchaigno pchaigno pchaigno approved these changes

@aditighag aditighag aditighag approved these changes

Assignees

@kkourt kkourt

Labels
kind/cleanup This includes no functional changes. 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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@chkhalt @pchaigno @aanm @kkourt @borkmann @aditighag