search

NLnetLabs / domain

A DNS library for Rust.
https://nlnetlabs.nl/projects/domain/about/
BSD 3-Clause "New" or "Revised" License
355 stars 60 forks source link

Improved zonefile parsing error messages. #362

Closed ximon18 closed 3 months ago

ximon18 commented 3 months ago

Produces more explanatory errors with supporting data (NS in this case), such as:

Attempted to add a normal record where a NS record already exists: barrique.de. 86400 IN A 5.35.254.208

Instead of this:

IllegalRecord: some.domain. 86400 IN A 188.40.131.90

Also makes the TryFrom<inplace::Zonefile> for Zonefile warn about and skip errors rather than aborting. Users who want more control should iterate over the inplace::Zonefile content themselves.

partim commented 3 months ago

I’m not a big fan of logging error information and then quietly discarding it in a library. I’d rather TryFrom would return a vec of all the error if it fails and there‘d be a method to explicitly clear all errors.

ximon18 commented 3 months ago

I’m not a big fan of logging error information and then quietly discarding it in a library. I’d rather TryFrom would return a vec of all the error if it fails and there‘d be a method to explicitly clear all errors.

I can do this, but it will cause a couple of problems, especially for larger zones, namely increased memory usage and a (big) delay before the issues are reportable back to the end user. An alternative could be to support a callback for user defined handling while the data is being parsed. However, on reflection, this is just a helper fn and users who need more fine grained control can iterate over the records themselves and log or abort however they like.

ximon18 commented 3 months ago

I’m not a big fan of logging error information and then quietly discarding it in a library. I’d rather TryFrom would return a vec of all the error if it fails and there‘d be a method to explicitly clear all errors.

It could be more memory efficient, but I think this approach is only suited to smaller zones anyway and larger zone users should not use the TryFrom converters but implement the steps themselves.

I've updated the PR to return the encountered errors as is already done for conversion to a ZoneBuilder, and standardized the two conversions to use the same ZoneErrors type to report a set of zone related errors.

I realize now that I'm not sure what you mean by clearing all errors however so I haven't done anything relating to that.

partim commented 3 months ago

I realize now that I'm not sure what you mean by clearing all errors however so I haven't done anything relating to that.

This was a misunderstanding of the code structure on my part. So, er, never mind …