starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #17] Convert all errors to use error-chain!{..} #311

New issue

Closed

opened 2026-03-15 21:53:36 +03:00 by kerem · 4 comments

kerem commented

2026-03-15 21:53:36 +03:00

Owner

Originally created by @bluejekyll on GitHub (Jun 15, 2016).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/17

Originally assigned to: @bluejekyll on GitHub.

This should simplify error handling in Trust-DNS

Originally created by @bluejekyll on GitHub (Jun 15, 2016). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/17 Originally assigned to: @bluejekyll on GitHub. This should simplify error handling in Trust-DNS

kerem

2026-03-15 21:53:36 +03:00

closed this issue
added the
enhance
label

kerem commented

2026-03-15 21:53:53 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jun 15, 2016):

https://users.rust-lang.org/t/announcing-error-chain-a-library-for-consistent-and-reliable-rust-error-handling/6133

@bluejekyll commented on GitHub (Jun 15, 2016): https://users.rust-lang.org/t/announcing-error-chain-a-library-for-consistent-and-reliable-rust-error-handling/6133

kerem commented

2026-03-15 21:53:58 +03:00

Author

Owner

@dckc commented on GitHub (Nov 6, 2016):

There are various uses of .expect() in named.rs for things that are ordinary runtime error conditions (I/O Error loading the config file, bad zone name, ...). Shouldn't panic!() and hence .expect() be limited to bugs? i.e. design-time problems?

I have to admit I can't figure out an alternative. I got the impression

fn load_zone(zone_dir: &Path, zone: &ZoneConfig) -> Result<Authority, String> {
  let zone_name: Name = zone.get_zone().expect("bad zone name");

should become

fn load_zone(zone_dir: &Path, zone: &ZoneConfig) -> Result<Authority, String> {
  let zone_name: Name = try!(zone.get_zone().chain_err("bad zone name"));

but it doesn't compile, even with use trust_dns::error::ParseChainErr added.

p.s. I'm commenting here rather than raising a new issue because I'm not sure whether this is as-designed or not.

@dckc commented on GitHub (Nov 6, 2016): There are various uses of `.expect()` in named.rs for things that are ordinary runtime error conditions (I/O Error loading the config file, bad zone name, ...). Shouldn't panic!() and hence .expect() be limited to bugs? i.e. design-time problems? I have to admit I can't figure out an alternative. I got the impression ``` rust fn load_zone(zone_dir: &Path, zone: &ZoneConfig) -> Result<Authority, String> { let zone_name: Name = zone.get_zone().expect("bad zone name"); ``` should become ``` rust fn load_zone(zone_dir: &Path, zone: &ZoneConfig) -> Result<Authority, String> { let zone_name: Name = try!(zone.get_zone().chain_err("bad zone name")); ``` but it doesn't compile, even with `use trust_dns::error::ParseChainErr` added. p.s. I'm commenting here rather than raising a new issue because I'm not sure whether this is as-designed or not.

kerem commented

2026-03-15 21:54:03 +03:00

Author

Owner

@bluejekyll commented on GitHub (Nov 6, 2016):

I'll try and answer this question as best I can. First on the usage of expect/unwrap/panic in named. This is a top level binary. I've only expected it to be run through the main() method in binary form. To me, this means that given that any error in loading config will end up causing the application to exit abnormally, is fine. For anyone trying to use this as I library, where unwraps/expects should all be guarded against, I never intended them to call into named.rs (is that even possible?), but through the lib.rs and the trust_dns::server module.

Now, we could clean this up in named.rs for the purposes of outputting cleaner errors to end users, that's probably a good goal at some point, but that hasn't been a top priority for me. I'd absolutely accept patches to clean that up!

As to your issue with the chain_err(...) conversion issue, the Result probably has to be mapped to a String with map_err(...). Even though I've changed everything to use error-chain everywhere, I haven't actually used chain_err(...) enough to know exactly what the issue is there.

@bluejekyll commented on GitHub (Nov 6, 2016): I'll try and answer this question as best I can. First on the usage of expect/unwrap/panic in named. This is a top level binary. I've only expected it to be run through the main() method in binary form. To me, this means that given that any error in loading config will end up causing the application to exit abnormally, is fine. For anyone trying to use this as I library, where unwraps/expects should all be guarded against, I never intended them to call into `named.rs` (is that even possible?), but through the `lib.rs` and the `trust_dns::server` module. Now, we could clean this up in `named.rs` for the purposes of outputting cleaner errors to end users, that's probably a good goal at some point, but that hasn't been a top priority for me. I'd absolutely accept patches to clean that up! As to your issue with the `chain_err(...)` conversion issue, the Result probably has to be mapped to a String with `map_err(...)`. Even though I've changed everything to use error-chain everywhere, I haven't actually used `chain_err(...)` enough to know exactly what the issue is there.

kerem commented

2026-03-15 21:54:08 +03:00

Author

Owner

@dckc commented on GitHub (Nov 6, 2016):

OK, thanks for clarifying: panic!() from named should probably be cleaned up for nicer UX, though it's not critical. That's the way I look at it too.

Here's hoping for time to look into it further.

@dckc commented on GitHub (Nov 6, 2016): OK, thanks for clarifying: panic!() from named should probably be cleaned up for nicer UX, though it's not critical. That's the way I look at it too. Here's hoping for time to look into it further.

kerem referenced this issue

2026-03-16 02:02:09 +03:00

[PR #311] [MERGED] minor optimizations for deserialization #1360