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

Use x/net/ipv4/6 to construct oob info when writing packets #3278

Merged
merged 3 commits into from
Sep 18, 2021
Merged

Use x/net/ipv4/6 to construct oob info when writing packets #3278

merged 3 commits into from
Sep 18, 2021

Conversation

sudarshan-reddy
Copy link
Contributor

@sudarshan-reddy sudarshan-reddy commented Sep 15, 2021

Fixes #3275.

All Unit tests pass and existing unit tests in conn_oob_test.go already cover the use case addressed by this fix.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@sudarshan-reddy
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #3278 (e491b5c) into master (ebcd98e) will increase coverage by 0.22%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3278      +/-   ##
==========================================
+ Coverage   85.47%   85.69%   +0.22%     
==========================================
  Files         132      132              
  Lines        9799     9775      -24     
==========================================
+ Hits         8375     8376       +1     
+ Misses       1048     1024      -24     
+ Partials      376      375       -1     
Impacted Files Coverage Δ
conn_oob.go 71.88% <0.00%> (+11.35%) ⬆️
conn_windows.go 56.52% <0.00%> (ø)
internal/qtls/go117.go 58.33% <0.00%> (ø)
packet_handler_map.go 75.00% <0.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebcd98e...e491b5c. Read the comment docs.

@sudarshan-reddy sudarshan-reddy changed the title Used x/net/ipv4/6 to construct oob info when writing packets Use x/net/ipv4/6 to construct oob info when writing packets Sep 15, 2021
@marten-seemann
Copy link
Member

Just wondering, did you check if this works on FreeBSD? (not a requirement for this PR to be merged)

@sudarshan-reddy
Copy link
Contributor Author

I only verified that it did not break anything.

Unfortunately I don't have a freeBSD setup behind anycast. I'm going to set one up this weekend and can check this in a follow up. I propose if the PR is good, we can merge it but keep the issue open until I can test FreeBSD.

conn_oob.go Outdated Show resolved Hide resolved
conn_oob.go Outdated Show resolved Hide resolved
oob := make([]byte, cmsglen)
cmsg := (*syscall.Cmsghdr)(unsafe.Pointer(&oob[0]))
cmsg.Level = syscall.IPPROTO_TCP
cmsg.Type = msgTypeIPv4PKTINFO
Copy link
Member

Choose a reason for hiding this comment

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

Can you delete this constant? Note it’s defined a couple of times, for the different build architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is still being used here: https://github.com/lucas-clemente/quic-go/blob/master/conn_oob.go#L174. I will delete it with my next PR that uses the x package also for reading OOB.

@sudarshan-reddy
Copy link
Contributor Author

home/circleci/project/integrationtests/self/key_update_test.go:101 failed for go117 but passed for go116. Is this flakey?

@sudarshan-reddy
Copy link
Contributor Author

sudarshan-reddy commented Sep 17, 2021

@marten-seemann CI LGTM. You okay if I squash commits?

Copy link
Member

@marten-seemann marten-seemann 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!

@marten-seemann marten-seemann merged commit c5aeee3 into quic-go:master Sep 18, 2021
@sudarshan-reddy sudarshan-reddy deleted the oob-with-net-structs branch September 18, 2021 20:27
saitolume pushed a commit to saitolume/quic-go that referenced this pull request Sep 20, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question about setting IP_PKTINFO for QUIC servers listening on wildcard (0.0.0.0).
3 participants