Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#2291 closed enhancement (implemented)

GETINFO Expansion

Reported by: atagar Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi. I've made the changes for some of the low hanging fruit for proposal 173 (getinfo-option-expansion). These are platform specific changes and mostly just concern the handling on *nix platforms (tested under 32bit Ubuntu 9.10). The one exception is the pid, where I'm fetching it via the same method as what we do for the PidFile.

Implemented:
"process/pid" -- Process id belonging to the main tor process.
"process/uid" -- User id running the tor process, -1 if unknown.
"process/user" -- Username under which the tor process is running, providing an empty string if none exists.
"process/descriptor-limit" -- File descriptor limit, -1 if unknown.

A few others from the proposal would be nice, but look more involved. If these are trivial then please give me a hint and I'll include them too:
"relay/read-total" -- Total bytes relayed (download).
"relay/write-total" -- Total bytes relayed (upload).
"process/descriptors-used" -- Count of file descriptors used.
"ns/authority" -- Router status info (v2 directory style) for all recognized directory authorities, joined by newlines.

Most of the other can be inferred from controllers so I'm not as interested in them. Providing the pid via the control port will also resolve:
https://trac.torproject.org/projects/tor/ticket/1388

Cheers! -Damian

PS. I'm using the PidFile as my example for cross-platform support, which might be unwise (it just splits between Windows/Non-Windows use cases). I'm not spotting anything that looks like an "#ifdef UNIX" in the tor code - is this the proper approach for cross-platform functionality?

Child Tickets

Attachments (3)

control.c_diff (2.8 KB) - added by atagar 8 years ago.
src/or/control.c changes
control-spec.txt_diff (685 bytes) - added by atagar 8 years ago.
control-spec changes for implemented options
0001-Implementing-getinfo-options-for-the-pid-uid-user-an.patch (4.2 KB) - added by atagar 8 years ago.
Changes from the first round of comments.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by atagar

Attachment: control.c_diff added

src/or/control.c changes

Changed 8 years ago by atagar

Attachment: control-spec.txt_diff added

control-spec changes for implemented options

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

comment:2 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:3 Changed 8 years ago by nickm

  • Instead of malloc + tor_snprintf, try using tor_asprintf. It works like malloc+sprintf, except that you never need to worry about the allocation size.
  • Some of the stuff you're leaving unimplemented on Windows totally exists, I think. See GetUserName(), for instance.
  • Over in compat.c , when we're actually doing all the rlimit work, we check for HAVE_GETRLIMIT, not MS_WINDOWS, and we give ourselves a descriptor limit on some platforms even if none is defined. Maybe the process/descriptor-limit code should have a look at the result of that, or refactor that code somehow so it can do both get and set?
  • The documentation should really say whether descriptor-limit is rlim_max or rlim_cur.

Changed 8 years ago by atagar

Changes from the first round of comments.

comment:4 Changed 8 years ago by atagar

Hi Nick. Thanks for the feedback!

Instead of malloc + tor_snprintf, try using tor_asprintf

Nice - done

Some of the stuff you're leaving unimplemented on Windows totally exists

As discussed on irc I don't think that it's practical for me to add Windows support without a test system to develop on. Also, I neither care about that platform nor have others expressed interest so I think that this is best left as a 'todo' until someone that cares about Windows support steps forward.

we check for HAVE_GETRLIMIT, not MS_WINDOWS

Fixed

and we give ourselves a descriptor limit on some platforms even if none is defined...

I've copied the constants from the set_max_file_descriptors. I agree that it would be nicer for compat.c to have a getter function, however in the HAVE_GETRLIMIT use case it uses both the rlim_max and rlim_cur so it's insufficient to have a get_max_file_descriptors function (in some cases we'd want an int and in others we want a struct).

The documentation should really say whether descriptor-limit is rlim_max or rlim_cur.

Specified that it's the upper limit.

comment:5 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay, it's tor time for me again.

I'm merging this, with an added comment about the duplicated code (we do want to fix that; duplicating constants across files is *begging* for them to get out of sync), and some whitespace fixes.

Also, there's no point in using U64_FORMAT and U64_PRINTF_ARG to print an int; %d is always adequate to print an int.

Thanks, and happy new year!

comment:6 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:7 Changed 7 years ago by nickm

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