-
Notifications
You must be signed in to change notification settings - Fork 205
Use new picoquic_iovect_t in alpn callback signature #2000
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
Conversation
huitema
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to have two separate calls: the old one for compatibility, and a new one using the "v2" signature.
I would implement that by having an additional value in the "picoquic_quic_t" struct:
picoquic_alpn_select_fn alpn_select_fn;
picoquic_alpn_select_fn_v2 alpn_select_fn_v2;
and then modify the code in tls_api.c from:
else if (quic->alpn_select_fn != NULL) {
size_t selected = quic->alpn_select_fn(quic, params->negotiated_protocols.list, params->negotiated_protocols.count);
if (selected < params->negotiated_protocols.count) {
alpn_found = params->negotiated_protocols.list[selected].base;
alpn_found_length = params->negotiated_protocols.list[selected].len;
ptls_set_negotiated_protocol(tls, (const char *)params->negotiated_protocols.list[selected].base, params->negotiated_protocols.list[selected].len);
}
}
to:
else {
size_t selected = params->negotiated_protocols.count;
if (quic->alpn_select_fn != NULL) {
selected = quic->alpn_select_fn(quic, params->negotiated_protocols.list, params->negotiated_protocols.count);
}
else if (quic->alpn_select_fn_v2 != NULL) {
selected = quic->alpn_select_fn_v2(quic, (picoquic_iovec_t*)¶ms->negotiated_protocols.list, params->negotiated_protocols.count);
}
if (selected < params->negotiated_protocols.count) {
alpn_found = params->negotiated_protocols.list[selected].base;
alpn_found_length = params->negotiated_protocols.list[selected].len;
ptls_set_negotiated_protocol(tls, (const char *)params->negotiated_protocols.list[selected].base, params->negotiated_protocols.list[selected].len);
}
}
The current casting is not adequate -- it does break the windows builds.
Introduce picoquic_alpn_select_fn_v2 that uses picoquic_iovec_t instead of ptls_iovec_t. The old picoquic_alpn_select_fn remains backward compatible. If both are set, the older one takes precedence - Add picoquic_set_alpn_select_fn_v2() to set new-style callbacks - Update sample apps to use v2 API - Remove unneeded picotls.h and tls_api.h includes from demoserver
huitema
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Just a minor suggestion in case the developer mixes the two APIs.
Make sure the latest update wins!
This seems cleaner than casting in the alpn callback, but could be a breaking change.