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

quiche: update HTTP/3 config creation to new API #4437

Closed
wants to merge 2 commits into from

Conversation

@LPardue
Copy link
Contributor

commented Sep 27, 2019

Depends on pending changes to quiche: 4a51aad615975a4051ea32e2d25104feac90c430 and 906646c43512efcd651f415b279cb89797c97fb8. See https://github.com/cloudflare/quiche/pull/180/commits for context

if(!qs->h3config)
return CURLE_OUT_OF_MEMORY;

quiche_h3_config_set_max_header_list_size(qs->h3config, 1024);

This comment has been minimized.

Copy link
@ghedo

ghedo Sep 27, 2019

Member

I think we can omit this. Hardcoding the limit here is not very useful and AFAICT the ngtcp2 backend doesn't set a limit, so we should probably be consistent.

This comment has been minimized.

Copy link
@LPardue

LPardue Sep 27, 2019

Author Contributor

Make sense. For anyone not tracking H3 specs closely, if an endpoint doesn't set a HTTP_MAX_HEADER_LIST_SIZE it is treated as effectively unlimited. Constructing the Config object and not setting anything will invoke that behaviour.

@LPardue LPardue force-pushed the LPardue:quiche-h3-config branch from 57e1bed to 0dc3e17 Sep 27, 2019
@bagder bagder added the HTTP/3 label Sep 28, 2019
@bagder
bagder approved these changes Sep 28, 2019
lib/vquic/quiche.c Outdated Show resolved Hide resolved
Co-Authored-By: Alessandro Ghedini <alessandro@ghedini.me>
@ghedo

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

The quiche PR was merged, so to avoid breaking other curl builds I squashed and pushed this change manually as 19338e9 even if the builds are not finished yet (I tested this locally first). If something breaks I'll look into preparing a fix.

@ghedo ghedo closed this Sep 29, 2019
@ghedo

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

Aaaaand I forgot to add "Closes: ..." to the commit message 🤦‍♂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.