Opened 11 years ago

Last modified 8 years ago

#1025 closed defect (Won't implement)

Completion of error handling

Reported by: elfring Owned by:
Priority: Low Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: elfring, nickm, Sebastian Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Some checks for return codes are missing.

Would you like to add more error handling for return values from "pthread_cond_signal" like in the function
"tor_cond_signal_one" and from "fprintf" in the function "write_pidfile"?

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (18)

comment:1 Changed 11 years ago by nickm

Yes, more code to check return values would be fine. I'm not sure about the write_pidfile() case: What _is_ the
reasonable behavior when fprintf() fails in that case?

comment:2 Changed 11 years ago by elfring

I suggest to avoid unchecked function calls.
Would you like to detect every error situation as early as possible?

How do you think about the reaction "exit(errno)" or "abort()"?

I would like point out another open issue with the calls "pthread_mutexattr_init" and "pthread_mutexattr_settype" in the function "tor_threads_init".

Would you like to reduce the efforts for error code checking by an exception class hierarchy?

comment:3 Changed 11 years ago by nickm

1) There is no way that exit() or abort() is an appropriate response to getting a write error on a pidfile. Stopping

the entire process is really only appropriate in the presence of an unrecoverable error that makes Tor not work. I
don't think being unable to write a pid file really rises to this level.

2) You say you'd like to point out an open issue with the mutexattr functions, but you don't say what that issue is.

Do you mean that they should get their return values checked? Or something else?

3) I don't think porting Tor to this weird exception library would help error code checking. The functions that you're

talking about (pthread_*, fprintf, etc) are already native C functions, so in cases where it's important to check
their output, we would _already_ need to add checks. Also, the exception library you link to has really poor error
recovery properties: unlike C++ exceptions, it doesn't do anything to clean up resource as it unwinds the stack.

comment:4 Changed 11 years ago by elfring

1) How do you think about to change the function interface so that it returns a value for the result?
Are you sure to ignore the failure for a file write access?

2) Return values should never be ignored.

3) (C++) exceptions provide a means to support more powerful error reporting.
Will all function implementations stick to a traditional C coding style forever?

Usual design choices are described in greater detail in the article "Exception Handling Alternatives" by Detlef Vollmann.

Are there any chances to apply usual advices by tools from the software area "aspect-oriented programming"?

comment:5 Changed 11 years ago by nickm

1) Returning a value and/or warning is fine.

2) So you _did_ mean the return values of those two functions, and not something else. Ok.

(Actually, it is fine to ignore return values in cases where the action you want to take on failure is the

same as the action you want to take on success. This isn't the case here, but the rule you propose is

3) There are no current plans to rewrite Tor in C++.

comment:6 Changed 11 years ago by elfring

1) I am curious which refactorings will be acceptable for the current source files to complete the error handling.

Can you accept API adjustments for the source code?

2) How do you think about ignored return values from calls of the function "(f)close"?

3) Are there any chances to integrate a tool like "AspectC++" into the software build process?

How do you think about to write filters for specific function interfaces?

comment:7 Changed 11 years ago by arma

Please feel free to send patches for specific errors. We aren't going to do an
overhaul to switch to aspectc or other things like that.

Closing the bug entry.

comment:8 Changed 11 years ago by arma

And, opened again. What's up?

comment:9 Changed 11 years ago by elfring

Do you find any wrapper libraries acceptable to handle unexpected return values?
Can any tools be reused to avoid the reinvention of a coding wheel for the handling of exceptional situations?

comment:10 Changed 11 years ago by nickm

I can't pronounce on every wrapper library on earth, but AspectC++ is out. There's no way we're going to add a
dependency on a special compiler in order to write Tor in a variant language that is no longer standard C. What
other "wrapper libraries" did you have in mind?

Tor currently handles its errors in the traditional C way, by having functions return a value to indicate an
error, and detecting the return values of the functions we call. Adding checks for specific errors would be
quite welcome. I don't personally see the need for big architectural changes to handle the examples you

comment:11 Changed 11 years ago by elfring

If you want to exclude C++ capabilities like the support for exception throwing, I imagine that you will need to repeat a similar infrastructure in plain C to complete the error handling.

comment:12 Changed 11 years ago by Sebastian

So did you have any patches? I think Roger made it clear that we won't
change the entire codebase to add in some external infrastructure, but
would be happy to fix actual problems.

comment:13 Changed 11 years ago by elfring

There are several approaches available to fix the reported open issues. The way to go depends on the consensus for programming tools that can be reused here.

comment:14 Changed 11 years ago by nickm

The consensus by people programming Tor seems to be that adding additional build dependencies (libraries or tools)
for error handling is unnecessary (especially ones that change C's semantics), as is redoing the program in C++.

Please also feel free to submit any actual patches to handle any actual unhandled error conditions correctly.

comment:15 Changed 11 years ago by elfring

If tools can be reused that provide a higher abstraction level than the plain C programming language, I see the potential for less manual work in the source code.
It would be convenient to encapsulate the error detection and corresponding exception handling as advices (which are supported by AspectC++).

comment:16 Changed 11 years ago by Sebastian

Seriously, this doesn't seem to be going anywhere. If you have
a real fix somewhere and want to provide it, please reopen/make
a new bugreport. Otherwise, this bug is dead.

comment:17 Changed 11 years ago by Sebastian

flyspray2trac: bug closed.

comment:18 Changed 8 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.