tcpcopy icon indicating copy to clipboard operation
tcpcopy copied to clipboard

Fix signal handler

Open elfring opened this issue 12 years ago • 15 comments

The functions "exit" and "tc_log_info" do not belong to the list of async-signal-safe functions.

I guess that a different program design will be needed for your function "signal_handler".

elfring avatar Apr 10 '13 16:04 elfring

The issue has been fixed according to your advice.

Thanks a lot and have a nice day :)

wangbin579 avatar Apr 11 '13 06:04 wangbin579

This issue was not completely fixed. The added variables "srv_settings" and "s_event_loop" are also not async-signal-safe. I recommend an approach that will work with the data type "sig_atomic_t".

elfring avatar Apr 11 '13 14:04 elfring

Many thanks. I have fixed the issue.

Please check this: https://github.com/wangbin579/tcpcopy/tree/iss92

wangbin579 avatar Apr 12 '13 03:04 wangbin579

This result looks better. But I find a modification of the attribute "event_over" still misplaced.

elfring avatar Apr 12 '13 04:04 elfring

Sorry, I could not catch up your meaning.Could you explain it in more details?

wangbin579 avatar Apr 12 '13 08:04 wangbin579

The type of the attribute "event_over" is "tc_atomic_t" which is equals to "volatile sig_atomic_t".

Why still misplaced?

wangbin579 avatar Apr 12 '13 09:04 wangbin579

I guess that it makes a relevant difference for software portability if an integer variable can be atomically modified depending on the detail if this item is referenced by itself or as an element within a bigger data structure.

elfring avatar Apr 12 '13 10:04 elfring

How about this time?

https://github.com/wangbin579/tcpcopy/tree/iss92

wangbin579 avatar Apr 15 '13 02:04 wangbin579

Do you really need the modification of two global variables in the signal handler? Is the change of the variable "intercept_sig" sufficient to indicate a predicate which will be evaluated at other source code places?

elfring avatar Apr 15 '13 12:04 elfring

The variable "event_over" is used to stop the event loop and print the statistical info for problem diagnosis. As for the variable "intercept_sig", it could be used to find the reason why the program "intercept" sometimes terminates abnormally.

wangbin579 avatar Apr 16 '13 12:04 wangbin579

I find that the setting of a single global variable should be sufficient within a signal handler.

Would you eventually like to consider also the other design options?

elfring avatar Apr 16 '13 16:04 elfring

How about this time?

https://github.com/wangbin579/tcpcopy/tree/iss92

wangbin579 avatar Apr 17 '13 02:04 wangbin579

I find it interesting that you added input parameter validation. How do you think about to use a symbol like "SIGUSR2" instead of the value "1"?

Another implementation detail remains. I guess that I can not really give you a definitive answer for the setting here if a global signal-safe variable must also be marked with the visibility attribute "static".

elfring avatar Apr 17 '13 10:04 elfring

How about this time?

https://github.com/wangbin579/tcpcopy/tree/iss92

A global signal-safe variable needs not to be marked with the visibility attribute "static".

See nginx source code:

sig_atomic_t          ngx_event_timer_alarm; 

wangbin579 avatar Apr 17 '13 10:04 wangbin579

The CERT Secure Coding Standards mention a different information for a strictly conforming program. It might be that attributes like "volatile static" are already provided by the data type definition "sig_atomic_t".

I have found also a few other implementation details for improvement.

  • Would you like to use the message "signal received: %s" together with a call to the function "strsignal"?
  • Is the variable "tc_over" checked quick enough? How do you think about to make it independent from a poll timeout?

elfring avatar Apr 17 '13 11:04 elfring