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

Dns over quic support: bis #62

Open
wants to merge 72 commits into
base: master
Choose a base branch
from

Conversation

VanStratum
Copy link
Contributor

Follow-up on #11.
Currently WIP

Connection & streams errors are now handled as prescribed by the
rfc.
Comment some implementation choices.
C-style prints switched to C++ streams.
Also correct a mistake in getting the target address: next_target() was
called at each udp send.
The double free was caused by a misuse of an unique_ptr which freed a
buffer owned by quicly. Replacing it by a vector make it work nicely.
uvw::UDPHandle::send takes ownership of the data only when it is passed
using an unique_ptr.
The connection is also properly freed, wich plugs a memleak.
This allows to send the connection close message as quickly as possible,
and potentially save some packet round trips.  E.g., on one particular
test instance, a 1-query burst goes from 23 udp packets exchanged to
only 12.
A segfault occured at quic connection close, when _quic_session was
reset, but packets where still comming.
An assert fail occured when receive_data was called on a close()'d quic
session.
Some extraneous prints removed.
@weyrick
Copy link
Contributor

weyrick commented Aug 3, 2021

Hi @VanStratum can you give us a status update here - are you still working on this and hoping to land it?

@VanStratum
Copy link
Contributor Author

VanStratum commented Aug 4, 2021 via email

@weyrick
Copy link
Contributor

weyrick commented Aug 5, 2021

Ok awesome, @saradickinson is interested in this work and helping land it. It would be great if we could get that done by the end of september. I can help review.

@VanStratum
Copy link
Contributor Author

Okay, I'm focused on finishing my master thesis at the moment, but I will have more time in about two weeks.

With an adequately defined call to bind(), the handle knows if it must send
over IPv4 or IPv6.
The session ending of DoQ and of the tcp-based mode is slightly
different, putting them in the same function resulted in obscure code.
Main change is the use of length field, in the same way as DoT or DNS
over TCP.
@VanStratum
Copy link
Contributor Author

It think the code is ready for review. It builds fine with the last version of quicly, and I just migrated it to the latest draft.

@weyrick
Copy link
Contributor

weyrick commented Sep 1, 2021

@VanStratum shouldn't this PR be against master and not the old quic branch?

@VanStratum
Copy link
Contributor Author

VanStratum commented Sep 2, 2021 via email

@weyrick
Copy link
Contributor

weyrick commented Jan 7, 2022

@VanStratum do you think this work is salvageable - can it be reopened against master branch?

@VanStratum VanStratum changed the base branch from dns-over-quic to master January 9, 2022 19:23
@VanStratum
Copy link
Contributor Author

I just moved it to point to master.
I will have to look at the latest draft to see what has changed in the last 4 iterations.

@weyrick
Copy link
Contributor

weyrick commented Jan 10, 2022

Nice! I'll start a review.

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