Opened 9 months ago

Closed 9 months ago

#23848 closed enhancement (implemented)

Replace all exit() calls with return code style

Reported by: hellais Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-mobile, 0.3.5-deferrable, review-group-24
Cc: darkk, brade, mcs, ahf, nickm, mike@… Actual Points:
Parent ID: #23684 Points:
Reviewer: Sponsor: Sponsor8

Description

Currently tor uses inside of various places the exit() call, but this is not nice when you are using it as a library as it leads to the whole app crashing, while we would rather just get an exception or a proper error code.

This involves grepping tor for exit() calls and replacing them with instead return code style returns.

Child Tickets

Change History (10)

comment:1 Changed 9 months ago by hellais

Type: defectenhancement

comment:2 Changed 9 months ago by mtigas

Cc: mike@… added

comment:3 Changed 9 months ago by nickm

I've gone over the code. I found 19 unproblematic uses of exit(), and 24 ones that we should fix. I annotated the good ones with "exit ok" and the bad ones with "XXXX bad exit" in 35746a9ee75608e7b2393e4519adca602bf2615f.

There are probably a few bad exits in particular that will account for most of the undesirable exit() calls: the ones in hibernate.c; a few of the ones in main.c; and one or two in config.c.

comment:4 Changed 9 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 9 months ago by nickm

Status: acceptedneeds_review

See branch exit_carefully in my public repository.

This branch handles all but a few cases that I had marked as XXXX bad exit. I'm hoping to add tests, and also to open tickets for the remaining cases.

(The remaining cases are all ones where IMO safe recovery is potentially difficult. I also added some "XXXX" comments for cases where options_act() is behaving poorly, but IMO those are separate bugs.)

comment:6 Changed 9 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:7 Changed 9 months ago by nickm

Sponsor: Sponsor8

comment:8 Changed 9 months ago by ahf

Status: needs_reviewmerge_ready

09562421589c055a6cb49c802a248f727390d158 and 35746a9ee75608e7b2393e4519adca602bf2615f looks good.

In 78cbced45cd244c8aab84e6fb8087b852769f5bc the documentation string around the renamed function tell_event_loop_to_run_external_code() is still talking about finishing instead of running external code.

f0c3b6238168e351835fdc64b5c93b84d6528870 looks good.

c82cc8acb5aa43b3d96490d927dcc622f2c825e3 looks good, but maybe we should remove this code if we notice that it is never used during this release cycle?

9bafc3b1efd64b19695cd78fb51070ff8d3939f5, c247a2df6b7f6b5d997ef9e5ccc25eaaf3918db8, 853e73e815c1be9c9e533160f803de56e7d21147, 1df43aff41eb8198712af0aa14ec2d574b26682c, 3c99b810a46c2aba2ea4108018e78bc8f6a19013, 90daa28df403472d4a6aa7067ce6ab3264cdbe55, c4a07b261b552c39b64c8d47f1ba75f4bb4f37d6, and a62f59f000f05ac5f5a7a22387007a1b8c2ee2a4 all looks good.

comment:9 Changed 9 months ago by nickm

Thanks for the review! I've commented on those issues and merged.

c82cc8acb5aa43b3d96490d927dcc622f2c825e3 looks good, but maybe we should remove this code if we notice that it is never used during this release cycle?

I've added an XXXX comment about that, but I'm not 100% sure we should: if there is some weird bug that makes us not actually exit, I bet it will be hard to find. In that case, it would be good to make sure that Tor didn't just keep running once it had been asked to stop.

comment:10 Changed 9 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.