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

feat: Make get work on IPv6 network #777

Merged
merged 3 commits into from
Feb 21, 2023
Merged

feat: Make get work on IPv6 network #777

merged 3 commits into from
Feb 21, 2023

Conversation

flub
Copy link
Contributor

@flub flub commented Feb 21, 2023

No description provided.

Maybe the way deciding it's not available is not great, but it's
probably good enough.

I also wish rust had a "skipped" test outcome.
@@ -53,7 +53,11 @@ async fn setup(opts: Options) -> Result<quinn::Connection> {

let tls_client_config = tls::make_client_config(&keypair, opts.peer_id, opts.keylog)?;
let mut client_config = quinn::ClientConfig::new(Arc::new(tls_client_config));
let mut endpoint = quinn::Endpoint::client("0.0.0.0:0".parse().unwrap())?;
let bind_addr = match opts.addr.is_ipv6() {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we do this on the provide side as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually nvm, this is only needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the provider or server endpoint already binds to whatever address you give on the commandline, so we already do. it's not like we need to call a different api for quinn, it's just that we need to have the socket bound to the same family as the server endpoint.

Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

lgtm, lets wait for @b5 to confirm this works for him

@flub flub merged commit c28a378 into main Feb 21, 2023
@flub flub deleted the flub/ipv6 branch February 21, 2023 18:42
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.

None yet

2 participants