kazu-yamamoto / dnsext

Extensible DNS libraries written purely in Haskell
59 stars 3 forks source link

TODO #136

Open kazu-yamamoto opened 1 year ago

kazu-yamamoto commented 1 year ago
archaephyrryx commented 1 year ago

@kazu-yamamoto In build.sh and test.sh, what is the cab executable being used? Is it an alias to cabal or something entirely different?

kazu-yamamoto commented 1 year ago

Oh. Sorry. You can use cabal.

cab is a wrapper of cabal. You can install it with cabal install cab. Note that cab uses the v1 commands.

archaephyrryx commented 1 year ago

I have some potential improvements in mind for the core API, including a semi-overhaul of the internal representation of Domain objects, and optimizations to their conversion to Presentation Format. Would this be an appropriate refactoring for this task? I can submit a separate demo PR or design document if that would be helpful.

kazu-yamamoto commented 1 year ago

Would you explain key ideas in more detail?

archaephyrryx commented 1 year ago

Viktor and I worked in 2020-2021 on a DNS implementation that he has given me the all-clear to go ahead and scavenge useful bits and pieces from, as necessary or practical.

I would have to review the code a bit more to see what else can be made useful, but that is a cursory rundown of the first steps that might make sense as far as what I was thinking. It would also have to be checked against upstream API changes and tinkered with a bit, but a lot of it could be used as inspiration rather than merely dropped in.

kazu-yamamoto commented 1 year ago

Domain in dnsext-types is not ByteString anymore.

data Domain = Domain
    { -- The representation format. Case-sensitive, escaped.
      representation :: ShortByteString
    , -- Labels in wire format. Case-sensitive, not escaped.
      wireLabels :: [ShortByteString]
    , canonicalLabels :: ~[ShortByteString]
    -- ^ Eq and Ord key for Canonical DNS Name Order.
    --   Lower cases, not escaped.
    --   https://datatracker.ietf.org/doc/html/rfc4034#section-6.1
    }

The IsRepresentation class might have room to be improved. I'm sorry but I would reject TH since we cannot maintain it. (I used to use TH but I gave up.)

If you still think your proposal is useful, please go ahead.

Note that I know you can write pure code of this area very well. After this issue, I would like to know whether or not you can write IO code.

archaephyrryx commented 1 year ago

I apologize if it may have been slightly confusingly worded. The key idea of the first bullet-point wasn't that Domain uses ByteString instead of ShortByteString, but rather, that that is the only logical field it contains (i.e. newtype instead of data). As in, the wire format is the canonical in-memory representation, rather than a computation based on a list of labels. It wouldn't be an issue to use a ShortByteString-based Domain, if that was the point you were raising.

The TH stuff is totally optional, I was just looking over the commit history and files to refresh my memory and it cropped up. It doesn't need to be brought in if you have concerns about that.

I do think that there is enough useful content to bring in, and will proceed with the proposal, with the caveats you mentioned.

kazu-yamamoto commented 1 year ago

The wire format should be ByteString. But we avoid to have a ByteString field because it causes memory fragmentation. This is based on the experience of concurrent-dns-cache which is heavily used in the real world.