Skip to content

Completion of error handling #852

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

Open
elfring opened this issue Feb 23, 2023 · 8 comments
Open

Completion of error handling #852

elfring opened this issue Feb 23, 2023 · 8 comments

Comments

@elfring
Copy link

elfring commented Feb 23, 2023

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

@wcawijngaards
Copy link
Member

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

elfring commented Feb 23, 2023

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

@wcawijngaards
Copy link
Member

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

elfring commented Feb 23, 2023

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

@wcawijngaards
Copy link
Member

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

elfring commented Feb 23, 2023

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

@wcawijngaards
Copy link
Member

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

elfring commented Feb 23, 2023

That is interesting to read.

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

jedisct1 added a commit to jedisct1/unbound that referenced this issue Mar 20, 2023
* nlnet/master:
  - iana portlist update.
  - Fix NLnetLabs#812, fix NLnetLabs#846, by using the SSL_OP_IGNORE_UNEXPECTED_EOF option   to ignore the unexpected eof while reading in openssl >= 3.
  - Fix ssl.h include brackets, instead of quotes.
  - Fix unbound-dnstap-socket test program to reply the finish frame   over a TLS connection correctly.
  - Fix for NLnetLabs#852: Completion of error handling.
  Changelog entry for issue NLnetLabs#825
  Improved comment
  Test cache update from serve-expired and client-subnet-always-forward
  ifdef CLIENT_SUBNET
  Fix issue NLnetLabs#825: interaction between ECS and serve-expired.
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