add std.dns #3877

lun-4 · 2019-12-09T22:30:57Z

based off zigdig

TODO:

add DNSRData.size() support for v4 and v6 addresse
add serialization of v4 and v6 addresses
get a known good packet without pointers to test serialization with

- strToType -> DNSType.fromStr - toDNSName -> DNSName.fromString - remove classToStr and typeToStr, use tagName builtin - remove Header.init - add test for DNSType.fromStr

- fix DNSName.size - fix Packet.init - make tests use new functions - fix str to type test

since DNSName.serialize exists, we don't need helper functions for this

- make Resource serializable - remove LabelComponentTag - add asserts when serializing

- rdata: remove opaque parameter from parseRData

- add Resource methods to Packet - rename parseRData to deserializeRData

- add DNSRData.size()

frmdstryr · 2019-12-10T13:42:22Z

Can this do async dns resoultion? (eg like https://github.com/c-ares/c-ares)

lun-4 · 2019-12-10T14:28:32Z

Domain resolution is not in the scope of std.dns, this implements the structs and serialization/deserialization of them. That would go into std.net.getAddressList (which was implemented in a sync manner, see #3538 )

- rdata: remove unused function

- dns.test: make callers give the wanted base64 buffer

- dns: fix addAditional() incrementing the wrong header field - dns: add test for size() methods on DNSName and Resource - dns: add a "good enough" generated packet to prevent regressions

jayschwa · 2019-12-11T04:36:21Z

Perhaps it should go under std.net.dns?

andrewrk

Thank you for pressing forward on one of the most important parts of the std lib. This pull request is very welcome.

Networking in the std lib is something that must necessarily be given extra scrutiny, so just as a heads up, networking contributions will probably see an unusually high amount of review feedback. I have a round of review feedback for you to look at here.

A couple more things:

I agree with @jayschwa about std.net.dns vs std.dns.
I would like to see an attempt at DNS resolution API without heap memory allocation. I think it is possible and desirable. So merging of this pull request will be blocking on that attempt, whether it is done by you, me, or someone else.

I also would tend to favor going about this in the other direction: writing the dns resolution code first, and then smoothing out the the data types and exposing them as an API.

I believe some of these types are relevant to the existing getAddressInfo that you noted; it would be a good test of these data types and enums if the existing dns resolution code was updated to use them.

Cheers @lun-4, thanks again for working on this.

andrewrk · 2020-01-07T17:10:03Z

lib/std/dns.zig

+    // QTYPE only, but merging under DNSType
+    // for nicer API
+
+    // TODO: add them back, maybe?


Can you make this comment explain what's going on here a bit more, with the commented out items? I don't know much about this topic, so this looks confusing to me.

Those are some DNS packet types that weren't relevant to getting a "resolver from domain to ip" working back when I first wrote it. I'll add support for those types for completeness, though.

Related to #2524 ?

lib/std/dns.zig

andrewrk · 2020-01-07T17:33:23Z

lib/std/dns.zig

+pub const ResourceList = std.ArrayList(Resource);
+const InError = std.io.SliceInStream.Error;
+pub const DNSDeserializer = std.io.Deserializer(.Big, .Bit, InError);
+pub const DNSError = error{


In general, for naming things, take the full namespace name into account:

std.dns.DNSError

So you can leave off DNS here and in other places in this file, since it's implied by being inside the dns.zig file. More idiomatic would be:

std.dns.Error

lib/std/dns.zig

andrewrk · 2020-01-07T17:54:08Z

lib/std/dns.zig

+/// Keep in mind DNSName's are not singularly deserializeable, as the names
+/// could be pointers to different bytes in the packet.
+/// (RFC1035, section 4.1.4 Message Compression)
+pub const DNSName = struct {


In this type, the labels field can point to allocated memory. One thing that API users will be wondering is, "what's the plan for managing this memory?" Generally, the documentation for a type which holds references to heap memory should explain the memory-management strategy that it is participating in. For example, is the memory fully expected to be handled by the user of the API? That's fine, but users need to know that, especially in the docs for the functions that allocate heap memory. Is the type capable of cleaning up its own memory? Then it probably needs a deinit() method.

lib/std/dns/rdata.zig

andrewrk · 2020-02-18T22:27:58Z

I'm closing since this hasn't seen any progress for over a month. You are absolutely welcome to re-open this when it's ready for another round of reviews - I'm just putting the ball back in your court. 🏀

lun-4 added 25 commits December 1, 2019 12:31

add std.dns

851ac9b

add tests, add missing structs/enums

f1d3f5a

dns: rename DNSQuestion to Question, DNSResource to Resource

ff0f5dd

dns: DNSHeader -> Header, DNSPacket -> Packet

afef178

dns: move helpers to functions inside the type

20e5043

- strToType -> DNSType.fromStr - toDNSName -> DNSName.fromString - remove classToStr and typeToStr, use tagName builtin - remove Header.init - add test for DNSType.fromStr

fix DNSType.fromStr

037fc9e

- fix DNSName.size - fix Packet.init - make tests use new functions - fix str to type test

remove auto-numbering off DNSType and DNSClass

e7e1bc6

fix description of inDeserial internal func

26370b7

move resourceSize to Resource.size

a45baec

Merge branch 'master' into dns-structs

9a1ab1e

add authority and additional resource lists to Packet.size

957d357

add DNSPacketRCode, use address to array

92cb68c

add explanation for ex-but-still-staying-workaround

bd1f716

add std.dns.RData

9b68a10

remove internal serialName

b2aaf3a

since DNSName.serialize exists, we don't need helper functions for this

fix DNSName.serialize

322340a

update docstrings

e0bca89

- make Resource serializable - remove LabelComponentTag - add asserts when serializing

remove OpaqueDNSRData in favor of a slice of u8

33877ca

Merge branch 'master' into dns-structs

c28f394

dns: std.fmt changes

c335a99

dns.test: add rdata parsing tests

3a472a8

- rdata: remove opaque parameter from parseRData

dns: decrease buffer size for base64 encode tests

767d829

dns: add draft test for rdata serialization

37c038d

- add Resource methods to Packet - rename parseRData to deserializeRData

dns: fix serialization of Resource's RDATA length

ee4beed

- add DNSRData.size()

add support for more types on DNSRData.size() and prettyRData

da0c464

lun-4 added 3 commits December 10, 2019 15:17

rdata: add A and AAAA support to size()

388a2a2

- rdata: remove unused function

add serialization of A and AAAA RDATA

a4fb64e

- dns.test: make callers give the wanted base64 buffer

dns: fix extra 2 unused bytes from Resource.size

337c35b

- dns: fix addAditional() incrementing the wrong header field - dns: add test for size() methods on DNSName and Resource - dns: add a "good enough" generated packet to prevent regressions

lun-4 marked this pull request as ready for review December 10, 2019 21:15

andrewrk requested changes Jan 7, 2020

View reviewed changes

lun-4 added 6 commits January 7, 2020 16:04

Merge branch 'master' into dns-structs

0d2f703

simplify DNSType.fromStr

34ccd1f

InvalidType => InvalidDnsType

2419fb3

remove Header.repr

349c488

rdata: fixes for std.fmt using tuples

82f4a38

dns: remove internal splitCount for DNSName.fromString

bb872f4

fengb reviewed Jan 8, 2020

View reviewed changes

lib/std/dns/rdata.zig Outdated Show resolved Hide resolved

lun-4 added 12 commits January 16, 2020 19:40

Merge branch 'master' into dns-structs

23f1c5c

dns.DNSError -> dns.Error

9396736

add formatting support to dns names

aecfeaa

add formatting support for dns rdata

ab89534

remove dns.rdata.prettyRData

df30abe

use better type for self in DNSName functions

e47b23a

Merge branch 'master' into dns-structs

fe7c1f3

add support for SRV types

bb1d126

add support for SRV serialization/deserialization

1801b13

add better comments around commented DNS types

45ce978

DNSType => Type, DNSClass => Class

7f9a9d8

rename dns.DNSName to dns.Name

b084687

fengb mentioned this pull request Jan 28, 2020

std.fmt.format — with generators #4059

Closed

10 tasks

andrewrk closed this Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add std.dns #3877

add std.dns #3877

lun-4 commented Dec 9, 2019 •

edited

Loading

frmdstryr commented Dec 10, 2019

lun-4 commented Dec 10, 2019

jayschwa commented Dec 11, 2019

andrewrk left a comment

andrewrk Jan 7, 2020

lun-4 Jan 7, 2020

daurnimator Jan 8, 2020

andrewrk Jan 7, 2020

andrewrk Jan 7, 2020

andrewrk commented Feb 18, 2020

add std.dns #3877

add std.dns #3877

Conversation

lun-4 commented Dec 9, 2019 • edited Loading

frmdstryr commented Dec 10, 2019

lun-4 commented Dec 10, 2019

jayschwa commented Dec 11, 2019

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Jan 7, 2020

Choose a reason for hiding this comment

lun-4 Jan 7, 2020

Choose a reason for hiding this comment

daurnimator Jan 8, 2020

Choose a reason for hiding this comment

andrewrk Jan 7, 2020

Choose a reason for hiding this comment

andrewrk Jan 7, 2020

Choose a reason for hiding this comment

andrewrk commented Feb 18, 2020

lun-4 commented Dec 9, 2019 •

edited

Loading