Skip to content
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

rs_fatal is overused #55

Closed
sourcefrog opened this issue Jan 11, 2016 · 2 comments
Closed

rs_fatal is overused #55

sourcefrog opened this issue Jan 11, 2016 · 2 comments

Comments

@sourcefrog
Copy link
Contributor

@sourcefrog sourcefrog commented Jan 11, 2016

librsync has a function/macro rs_fatal etc, to indicate severe errors or internal bugs, which calls abort().

However as a well-behaved library, it's probably better if it doesn't do that unless there's no safe way to cleanly unwind. Probably most uses should be pulled out and it should just return an error.

@sourcefrog sourcefrog changed the title rs_fatal is misleading rs_fatal is overused Jan 11, 2016
@sourcefrog
Copy link
Contributor Author

@sourcefrog sourcefrog commented Jan 16, 2016

Actually it turns out it was accidentally not fatal on non-gcc compilers, but this should be fixed by 18af8d0.

@dbaarda
Copy link
Member

@dbaarda dbaarda commented Feb 22, 2017

My thoughts on this are;

anything that is a programming error (violating "contract" pre/post/invariant conditions), such as calling a function with invalid arguments or corrupted data-structures, should be checked with assert statements. These should never happen for valid code, so we don't want to waste cpu cycles on them in a production release, but we want to catch them in debug builds.

Anything that is very rare and basically unrecoverable for the whole program, like malloc failures, should be rs_fatal.

Anything that is an error caused by external things beyond the program's control, like bad input data, or that break librsync in ways that shouldn't kill the whole program, should be caught and return an error code for the program to handle as it sees fit.

Note I fixed quite a few of these in pull #88. A quick search shows not many left that are not meeting the criteria above. The only ones left that need changing are things that are checking programming errors and thus could be replaced with asserts for efficiency. This means we don't call rs_fatal() in any places that would be considered "not well-behaved library behaviour" any more.

dbaarda added a commit to dbaarda/librsync that referenced this issue Oct 16, 2017
dbaarda added a commit that referenced this issue Oct 16, 2017
Fix #55 remove excessive rs_fatal() calls.
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