Skip to content

Drop secondary index #64

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

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Drop secondary index #64

merged 3 commits into from
Jun 15, 2023

Conversation

k0ekk0ek
Copy link
Contributor

@k0ekk0ek k0ekk0ek commented Jun 8, 2023

This PR drops the secondary index in favor of slightly more complicated domain name and string parsing, which are the only two data types (for now) that actually benefited from it. It also slows down RRTYPE conversion too (presumably), but there maybe a way to speed that back up again (the hash requires the length, which made sense because we had it freely available, but maybe another algorithm can be implemented that doesn't require it). Overall performance is around 5% better.

More importantly, it is now forbidden to use quotes around values other than domain names and text. The parser in NSD allows for quotes everywhere, which is why it was implemented like that before, but disallowing that is more sane and in line with BIND's behavior.

Use of 1h2m3s notation for TTLs, which is an NSD extension, is now only supported if pretty_ttls is enabled.

There's other changes too. Note that this is still a draft because the code needs to be cleaned up a little. I focused on getting something fast because many mechanics are updated that may be important for the work done on #23.

@k0ekk0ek
Copy link
Contributor Author

k0ekk0ek commented Jun 8, 2023

It's good to know that I've also removed any use of longjmp. I thought it was a little iffy to start with and now that the parse functions must validate it's quoted, contiguous or both (depending on the value) it made little sense to keep it around.

This was referenced Jun 12, 2023
@k0ekk0ek k0ekk0ek force-pushed the tokenicer branch 3 times, most recently from b77623f to 1249487 Compare June 14, 2023 15:30
@k0ekk0ek k0ekk0ek added this to the Release 0.1.0 milestone Jun 14, 2023
@k0ekk0ek k0ekk0ek force-pushed the tokenicer branch 2 times, most recently from 2d16c73 to 8361e2c Compare June 14, 2023 16:41
@k0ekk0ek k0ekk0ek marked this pull request as ready for review June 14, 2023 16:48
@k0ekk0ek
Copy link
Contributor Author

I think this is ready to be merged. This fixes a number of issues, the most important one being quotes around fields that must not be quoted are now disallowed. FORMAT.md has been updated to reflect that. Next to that, the secondary index is dropped (the reasoning for which can be found on #30) and the longjmp (originally added in #1) has been dropped too. Scanner performance has been improved significantly and numbers are a little better overall, so that's good. This will be a good base for incrementally improving data-type parsers (an example of which is #65) and adding thus far unsupported record types (SVCB, WKS, etc).

For TLD zones, the data will be mostly glue data (NS, DS, RRSIG and NSEC(3)), so data-types used in those fields will benefit us most. Names and RRTYPEs being at the top of the list.

I've also resolved the warnings on Windows and enabled the -Werror-like flag on that platform.

k0ekk0ek added 3 commits June 15, 2023 10:05
Drop secondary indexes to simplify tape operation and disallow placement
of quotes around values other than domain names and text.

Additionally, no attempt is made to parse symbols for which no symbolic
constants have been specified and use of pretty TTL notation, e.g. 1m2s,
is only allowed if the pretty_ttl option is enabled.

Fixes NLnetLabs#30.
Fixes NLnetLabs#31.
Fixes NLnetLabs#38.
Fixes NLnetLabs#50.
@k0ekk0ek k0ekk0ek merged commit 1b01b7f into NLnetLabs:main Jun 15, 2023
@k0ekk0ek k0ekk0ek deleted the tokenicer branch June 15, 2023 08:15
@k0ekk0ek k0ekk0ek mentioned this pull request Aug 4, 2023
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.

2 participants