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

TODO #136

Open
2 of 5 tasks
kazu-yamamoto opened this issue Sep 25, 2023 · 8 comments
Open
2 of 5 tasks

TODO #136

kazu-yamamoto opened this issue Sep 25, 2023 · 8 comments

Comments

@kazu-yamamoto
Copy link
Owner

kazu-yamamoto commented Sep 25, 2023

  • Documentation. All exposed functions and data should have manual. Some are missing. Some would be incorrect since we don't update them from the "dns" library.
  • Cleaning up code. dnsext-utils:DNS.RRCache and dnsext-iterative are just written once, not refactored at all. Probably the hold unnecessary code. Some parts are hard to read. Refactoring is necessary.
  • disable6NS (aka don't use IPv6 transport). dnsext-iterative uses this parameter but dns-do53 and dns-dox do not. The latter two should use this parameter.
  • Stress test for bowline. We should find resource leak, if any.
  • Performance test for bowline. We need to check the performance against unbound.
@archaephyrryx
Copy link

@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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

Would you explain key ideas in more detail?

@archaephyrryx
Copy link

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.

  • Domains are implemented as ByteString representing fully qualified wire format A labels, with robust support for various natural conversions and operations based around that definition
  • Rich set of Builder-centric utility functions for writing various types into presentation form
  • Boilerplate code that enables static conversion of literal strings (e.g. "example.com") into the corresponding Domain value using TH splices

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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

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.

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

No branches or pull requests

2 participants