add NULL record

[RyanGibb]

See https://www.rfc-editor.org/rfc/rfc1035#section-3.3.10

Adds a dependency on hex to dns for pretty printing hex

[hannesm]

Thanks for your contribution. The only question I have is why. Do you actually have a usecase for "NULL"? (I'm more interested in keeping the code base small than to support all resource records we can find.)

[RyanGibb]

Yes, my use case is transporting arbitrary bytes over DNS. For an example, see https://github.com/yarrick/iodine.

[hannesm]

CI is failing (Error (warning 8 [partial-match]): this pattern-matching is not exhaustive) -- did you try to compile the code locally? I'm not keen to add a dependency for hex (hint: look at the source, there is already hex decoding and encoding that may be used in null records).

The README states rather clearly that legacy/barely used RR types is not dealt with. There is since some time support for Unknown : I.t -> Txt_set.t with_ttl rr in the Rr_map -- what is the advantage of supporting NULL explicitly? (As far as I can tell from this PR, there's no plan to add support for NULL to the zone parser.) Similarly, I wonder what the use of LOC is, so far I haven't seen an application that uses it, or did I miss it?

[hannesm]

I'm fine with merging this, but there are a few things:

• code style: the codebase doesn't use @@ for a reason (hard to read), please remove its usage
• why is the type bytes - I find this rather hard to reason about, and would prefer either Cstruct.t or string (since once you have a Null.t in your hand, the value shouldn't change)
• the encode: what would happen if a buffer > 65535 is passed? I understand that other code paths (such as Txt) suffer from similar issues, but I'm not very happy about that
• as already mentioned, adding a new dependency on hex isn't the path forward, we should drop dependencies, not add them. There's no need for a to_string function, and pp could use Cstruct.hexdump_pp
• the documentation heading is Text records, it should be something more related to null!?

[RyanGibb]

CI is failing (Error (warning 8 [partial-match]): this pattern-matching is not exhaustive) -- did you try to compile the code locally?

I did, and am using it live now. I'll investigate.

I'm not keen to add a dependency for hex (hint: look at the source, there is already hex decoding and encoding that may be used in null records).

I'll take a look.

There is since some time support for Unknown : I.t -> Txt_set.t with_ttl rr in the Rr_map -- what is the advantage of supporting NULL explicitly?

I don't think I can create a NULL record with this, like I can with:

Dns.Rr_map.Null_set.singleton (Bytes.of_string reply)

And the advantage of NULL over TXT is that it allows binary data of arbitrary length.

(As far as I can tell from this PR, there's no plan to add support for NULL to the zone parser.)

I'm serving it dynamically at the moment, and don't have a use for reading it from a zonefile. I could take a look at adding it, but I don't think there's support for encoding binary in the current zonefile parsing.

[hannesm]

There is since some time support for Unknown : I.t -> Txt_set.t with_ttl rr in the Rr_map -- what is the advantage of supporting NULL explicitly?

I don't think I can create a NULL record with this, like I can with:

Dns.Rr_map.Null_set.singleton (Bytes.of_string reply)

And the advantage of NULL over TXT is that it allows binary data of arbitrary length.

"Arbitrary".

Wouldn't something like

Dns.Rr_map.(singleton (Unknown (Result.get_ok (I.of_int 10))) (Txt_set.singleton "foobar")) 

Ah, for encoding purposes you'd need to prefix the length to the value. Indeed, that's a bit inconvenient.

(As far as I can tell from this PR, there's no plan to add support for NULL to the zone parser.)

I'm serving it dynamically at the moment, and don't have a use for reading it from a zonefile. I could take a look at adding it, but I don't think there's support for encoding binary in the current zonefile parsing.

It is fine to not include it in the zone file parser. But there is support for "encoding binary", as suggested in the RFC, using hex encoding (and TYPENNN as type).

But now I've done enough reviewing on this PR, and won't be back for the rest of the week.

[RyanGibb]

I'm fine with merging this, but there are a few things:

• code style: the codebase doesn't use @@ for a reason (hard to read), please remove its usage

I've removed any instances of @@ in my PR, although the reason I thought it was appropriate was due to a number of other uses [0].

• why is the type bytes - I find this rather hard to reason about, and would prefer either Cstruct.t or string (since once you have a Null.t in your hand, the value shouldn't change)

I've changed this to a Cstruct.t.

• the encode: what would happen if a buffer > 65535 is passed? I understand that other code paths (such as Txt) suffer from similar issues, but I'm not very happy about that

    let max_len = 65535 in
    let len = min max_len (Cstruct.length null) in
    Cstruct.blit null 0 buf off len ;

Would copy the first 65535 octets from buf. We could throw an error instead.

• as already mentioned, adding a new dependency on hex isn't the path forward, we should drop dependencies, not add them. There's no need for a to_string function, and pp could use Cstruct.hexdump_pp

Done!

• the documentation heading is Text records, it should be something more related to null!?

And also fixed.

[0] Instances of @@

$ grep -r @@
app/odns.ml:          String.concat " " (List.rev @@ String.sub hex n (hlen-n)::acc)
test/tests.ml:    let encoded = fst @@ encode `Udp res in
test/tests.ml:                (Ok res) (decode @@ encoded))
test/tests.ml:                (Ok res) (decode @@ fst @@ encode `Udp res))
test/tests.ml:                data' (fst @@ encode `Tcp res));
test/tests.ml:    let _ = Format.printf "%a\n" Cstruct.hexdump_pp (fst @@ encode `Udp res) in
test/tests.ml:    let _ = Format.printf "%a" Cstruct.hexdump_pp (fst @@ encode `Udp res) in
test/tests.ml:    Alcotest.(check (result t_ok p_err) "Loc encodes" (Ok res) (decode @@ fst @@ encode `Udp res))
test/client.ml:          mail_exchange = Domain_name.host_exn @@ Domain_name.of_string_exn domain_name
test/client.ml:      Alcotest.(check bool __LOC__ true @@ Dns.Rr_map.Mx_set.equal mx_set @@ Dns.Rr_map.Mx_set.of_list @@
eio/client/dns_client_eio.mli:        Eio_main.run @@ fun env ->
eio/client/dns_client_eio.mli:        Dns_client_eio.run @@ fun stack ->
eio/client/ohost.ml:    msgf @@ fun ?header ?tags fmt -> with_stamp header tags k ppf fmt
eio/client/ohost.ml:  Eio_main.run @@ fun env ->
eio/client/ohost.ml:  Dns_client_eio.run env @@ fun stack ->
eio/client/dns_client_eio.ml:    ; timeout = Eio.Time.Timeout.v stack.mono_clock @@ Mtime.Span.of_uint64_ns timeout
eio/client/dns_client_eio.ml:      let ns = to_ip_port @@ nameserver_ips t in
eio/client/dns_client_eio.ml:              (to_ip_port @@ nameserver_ips t)
eio/client/dns_client_eio.ml:          let packet, recv_data = Cstruct.split recv_data @@ packet_len + 2 in
eio/client/dns_client_eio.ml:          if not @@ IM.is_empty ctx.requests then handle_data recv_data else ()
eio/client/dns_client_eio.ml:      if not @@ IM.is_empty ctx.requests then

[hannesm]

thanks. about @@ - you seem to use some branch that is not main for grepping. the eio-dns-client PR is still work in progress as far as I understand.