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

Expose C api to handle PathEvent #1660

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

normanmaurer
Copy link
Contributor

Motivation:

There was no C api exposed to allow handling PathEvent.

Modifications:

Expose c functions to handle PathEvent

Result:

Be able to handle and consume PathEvent

@normanmaurer normanmaurer requested a review from a team as a code owner November 20, 2023 14:43
@normanmaurer normanmaurer force-pushed the c_api_path_event branch 4 times, most recently from 6f8b6aa to 8a3d847 Compare November 20, 2023 15:42
@normanmaurer
Copy link
Contributor Author

I still need to test this a bit more but wanted to leave this here to understand if the API is ok or not /cc @ghedo @LPardue

@normanmaurer normanmaurer force-pushed the c_api_path_event branch 5 times, most recently from 3423f50 to 51f57c5 Compare November 21, 2023 07:54
@normanmaurer
Copy link
Contributor Author

So this seems to work... just tested it with my integration code

normanmaurer added a commit to netty/netty-incubator-codec-quic that referenced this pull request Nov 21, 2023
Motivation:

How we used QuicConnectEvent to notify about connection migration was not correct at all. Let's remove it for now as we will introduce something better once cloudflare/quiche#1660 was merged into quiche.

Modifications:

Remove QuicConnectEvent

Result:

Get rid of incorrect implementation to prepare for adding a proper one
normanmaurer added a commit to netty/netty-incubator-codec-quic that referenced this pull request Nov 21, 2023
Motivation:

How we used QuicConnectEvent to notify about connection migration was
not correct at all. Let's remove it for now as we will introduce
something better once cloudflare/quiche#1660 was
merged into quiche.

Modifications:

Remove QuicConnectEvent

Result:

Get rid of incorrect implementation to prepare for adding a proper one
@normanmaurer normanmaurer force-pushed the c_api_path_event branch 2 times, most recently from e8676b4 to b99516c Compare November 22, 2023 11:36
@normanmaurer
Copy link
Contributor Author

@LPardue all addressed

@normanmaurer normanmaurer requested a review from LPardue November 23, 2023 19:46
@LPardue
Copy link
Contributor

LPardue commented Nov 24, 2023

There's a lot of passing pairs of

foo: &mut sockaddr_storage,
foo_len: &mut socklen_t

wondering if maybe we should define a struct...

@normanmaurer
Copy link
Contributor Author

normanmaurer commented Nov 24, 2023

There's a lot of passing pairs of

foo: &mut sockaddr_storage,
foo_len: &mut socklen_t

wondering if maybe we should define a struct...

I was thinking about it but decided against it as other functions that take those pairs dont use a struct as well. That said I am fine with adding a struct... Maybe lets wait for @ghedo to chime in

@normanmaurer normanmaurer force-pushed the c_api_path_event branch 2 times, most recently from 3980108 to 7b54fa1 Compare November 24, 2023 11:59
@normanmaurer
Copy link
Contributor Author

rebased

Copy link
Member

@ghedo ghedo left a comment

Choose a reason for hiding this comment

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

Added a nit, I'll apply it myself when merging.

Motivation:

There was no C api exposed to allow handling PathEvent.

Modifications:

Expose c functions to handle PathEvent

Result:

Be able to handle and consume PathEvent
@ghedo ghedo merged commit 5796adc into cloudflare:master Nov 27, 2023
@normanmaurer normanmaurer deleted the c_api_path_event branch December 8, 2023 19:55
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.

3 participants