NLnetLabs / unbound

Unbound is a validating, recursive, and caching DNS resolver.
https://nlnetlabs.nl/unbound
BSD 3-Clause "New" or "Revised" License
3.12k stars 359 forks source link

Completion of error handling #852

Open elfring opened 1 year ago

elfring commented 1 year ago

Would you like to add more error handling for return values from functions like the following?

wcawijngaards commented 1 year ago

The strdup call is nicer with error handling. It is added in the commit above. The prints in the usage printout routine from unbound-control do not need such error handling, I would think.

Thanks for the heads up on the error oversight in the test program! That is nice to have a fix for.

elfring commented 1 year ago

I suggest to avoid ignorance of return values a bit more. Would you like to detect every error situation as early as possible?

wcawijngaards commented 1 year ago

I think it is a good idea to avoid ignorance of return values. But for such print statements, in the usage output, I think that is not nicer to check them.

There are a number of (void) casts that are used when return values are not inspected, already in the code. So that coverage can be extended. The value of this is debatable, it turns maybe into too many false positives and clutter. Or maybe it is useful as an annotation.

elfring commented 1 year ago

:thought_balloon: I propose to take also the possibility better into account for data output failures.

wcawijngaards commented 1 year ago

Yes, it is important to check for data output failures. For that, checking the return value of print function when NSD is printing to file, in other places, is important. But the usage error text data output is not significant.

The annotation, if useful for analysis, could prove useful. That could be a reason to add it.

Of course, we want the code to be as good as possible. I am not sure how this change would impact benefits from that programming paradigm.

elfring commented 1 year ago

:thinking: Can you become more aware that error detection belongs to known cross-cutting concerns?

wcawijngaards commented 1 year ago

Yes, that is true, that error handling fits in that way. That is interesting to read. :-)

For the usage printout, I think the result of handling the errors would make for annotated non-handling of it. Handling it to then try to printout another error that error printout does not work is not an improvement in program functionality for the usage printout, I think.

elfring commented 1 year ago

That is interesting to read.

:eyes: Some development approaches are worth for another look, aren't they?