Opened 8 months ago

Closed 8 months ago

#23845 closed enhancement (implemented)

Document a stable tor main function

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, review-group-24, s8-api
Cc: darkk, brade, mcs, nickm, ahf, mike@…, sbs Actual Points:
Parent ID: #23684 Points:
Reviewer: Sponsor: Sponsor8

Description

For the usage in the mobile context it's useful to have a documented tor main function to call to start it and exposed as C headers.

Child Tickets

Change History (14)

comment:1 Changed 8 months ago by hellais

Type: defectenhancement

comment:2 Changed 8 months ago by mtigas

Cc: mike@… added

comment:3 Changed 8 months ago by hellais

Cc: sbs added

comment:4 Changed 8 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 8 months ago by nickm

Status: acceptedneeds_review

See branch tor_api in my tor repository. Probably needs some changes!

comment:6 Changed 8 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:7 Changed 8 months ago by nickm

Sponsor: Sponsor8

comment:8 Changed 8 months ago by ahf

Status: needs_reviewmerge_ready

72b5e4a2db4282002fe50e11c2b8a79e108d30f8 looks fine.

In 8dd957cb8ae5c82da576e7720b1bb3b846089f98 the comment:

 * This interface is intended for use by programs that need to link Tor as
 * a library, and launch it in a separate thread.  If you have the ability
 * to run Tor as a separate process, you probably should.
 **/

reads a bit to me that this API is only useful if you have Tor running in a thread (but not as a process) - isn't it useful in both cases?

Otherwise this commit looks good as well. Nice with a documented, public, header! :-)

a7c126f65438a8dcde5663a731f0f8528e480bcc looks good.

comment:9 Changed 8 months ago by nickm

isn't it useful in both cases?

If you can run Tor as a separate process, I think you should probably just use exec*() to launch it, rather than having it linked in as part of your binary. I've tried to clarify in a fixup commit. Does it look better now?

Also, I think I should wait for the mobile people to comment too, so we know if they like the API

comment:10 Changed 8 months ago by ahf

Yep, the fixup looks good.

Let's await and see what the mobile people say.

comment:11 Changed 8 months ago by ahf

Keywords: s8-api added

comment:12 Changed 8 months ago by hellais

sbs and I looked into this and agree that it looks good.

Some minor nitpicks and comments we have are the following:

  • What is the purpose of tools/tor_runner.c? We suspect it's some sort of test to see if the API works, but were unclear about what it's exact purpose (and usage) is.
  • It would be useful to maybe add to the docstring of tor_run_main explaining how to construct tor_main_configuration_t
  • Related to the previous point, it would be useful to document what are the "best practices" in terms of initial configuration options to pass to tor_run_main when used as part of a library (for example, setting OwningControllerProcess or not). Good pointers can be found in txtorcon: https://github.com/meejah/txtorcon/blob/master/txtorcon/controller.py#L76.

I don't think these are blocking to merging this branch, but if you consider these things useful, maybe it would be appropriate to file some tickets for them (if you would rather postpone this).

Thanks for the quick turnaround on this, it's greatly appreciated!

comment:13 in reply to:  12 Changed 8 months ago by nickm

Replying to hellais:

sbs and I looked into this and agree that it looks good.

Some minor nitpicks and comments we have are the following:

  • What is the purpose of tools/tor_runner.c? We suspect it's some sort of test to see if the API works, but were unclear about what it's exact purpose (and usage) is.

Good point -- I should add some comments. It is meant to make a library that provides the same API as tor_run_main(), but which launches Tor in a separate process, so that you can write code that will work either way.

  • It would be useful to maybe add to the docstring of tor_run_main explaining how to construct tor_main_configuration_t

Will do.

I think this will have to be a separate ticket. Many of those ideas should IMO become other options that you can set on tor_main_configuration_t.

I don't think these are blocking to merging this branch, but if you consider these things useful, maybe it would be appropriate to file some tickets for them (if you would rather postpone this).

Thanks for the quick turnaround on this, it's greatly appreciated!

comment:14 Changed 8 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Fixed and merged to master!

Note: See TracTickets for help on using tickets.