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

Pass remote_port to MsH3ConnectionOpen #8762

Closed
wants to merge 2 commits into from
Closed

Pass remote_port to MsH3ConnectionOpen #8762

wants to merge 2 commits into from

Conversation

unasuke
Copy link
Contributor

@unasuke unasuke commented Apr 27, 2022

What

MsH3 supported additional "Port" parameter to connect not hosted on 443 port QUIC website.

image

lib/vquic/msh3.c Outdated
@@ -95,7 +95,7 @@ static const MSH3_REQUEST_IF msh3_request_if = {

void Curl_quic_ver(char *p, size_t len)
{
(void)msnprintf(p, len, "msh3/%s", "0.0.1");
(void)msnprintf(p, len, "msh3/%s", "0.3.0");
Copy link
Member

Choose a reason for hiding this comment

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

This seems brittle, can we persuade the good msh3 folks to add version information to msh3.h which we can consume?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree! That's why I created nibanks/msh3#23. Feel free to take a crack at it.

Copy link
Contributor Author

@unasuke unasuke Apr 28, 2022

Choose a reason for hiding this comment

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

RIght. it's useful if version string API in mash3. @nibanks What do you think about?

Copy link
Member

Choose a reason for hiding this comment

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

As nibanks/msh3#38 has been merged now, this can now get the runtime version!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! I'll try to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bagder I addressed it at deb1bc3

image

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM!

qs->conn = MsH3ConnectionOpen(qs->api,
conn->host.name,
(uint16_t)conn->remote_port,
unsecure);
Copy link
Member

Choose a reason for hiding this comment

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

This will put a hard requirement on a very recent version of the library, that should go into the --with-msh3 autoconf check to test for this signature no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's very safe if there is a signature checking, but too early to introduce it probably.

Copy link
Member

Choose a reason for hiding this comment

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

I think for now it is fine to require a very recent version of msh3 to build. It is early days and I assume it might move a bit going forward. We've worked similar to that with the other h3 libraries. They're all "beta" and this is functionality still labeled "experimental" in curl.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Contributor

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@unasuke
Copy link
Contributor Author

unasuke commented Apr 29, 2022

It is unlikely that I will be able to get this to pass all the tests...

@bagder
Copy link
Member

bagder commented Apr 29, 2022

It is unlikely that I will be able to get this to pass all the tests...

I'm afraid you see a lot of unrelated flaky CI build failures right now. They're not happening because of anything in this PR.

@bagder
Copy link
Member

bagder commented Apr 30, 2022

Thanks!

@bagder bagder closed this in 279dfb6 Apr 30, 2022
bagder pushed a commit that referenced this pull request Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants