[tor-bugs] #29221 [Core Tor/Tor]: Tooling to track code-style/best-practices violations
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Feb 26 12:09:59 UTC 2019
#29221: Tooling to track code-style/best-practices violations
--------------------------------------------+------------------------------
Reporter: nickm | Owner: (none)
Type: defect | Status: new
Priority: Medium | Milestone:
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: network-team-roadmap-2019-Q1Q2 | Actual Points:
Parent ID: | Points: 3
Reviewer: | Sponsor:
| Sponsor31-can
--------------------------------------------+------------------------------
Comment (by asn):
OK I'm getting started with this ticket:
Nick what do you mean that some of these measures already have code for
them? We have code in Tor to count function and file length? I could not
find that in `scripts/` but we do have the `#include` checking script.
On this note, and since we are chasing for low-hanging fruit here, I
played a bit with static analysis tools today. The thing that worked out
best because it's open source and does some of the things that we want it
do is GNU complexity (...): https://www.gnu.org/software/complexity/
Based on the best practices list of #29219, complexity can calculate: file
length, function length, nesting level count, and function-per-file count.
As far as I know we don't have any of these right now so being able to do
these would be an improvement. Also it's a GNU tool so it's FOSS and
something we can use as an optional parts of our scripts toolkit.
As an example here is complexity's output for `hs_intropoint.c`:
{{{
$ complexity --histogram --score --thresh=1 ./src/core/or/scheduler_kist.c
NOTE: proc kist_scheduler_run in file ./src/core/or/scheduler_kist.c line
544
nesting depth reached level 5
Complexity Scores
Score | ln-ct | nc-lns| file-name(line): proc-name
1 2 2 ./src/core/or/scheduler_kist.c(140):
free_all_socket_info
1 4 2 ./src/core/or/scheduler_kist.c(420): MOCK_IMPL
1 4 2 ./src/core/or/scheduler_kist.c(473):
kist_scheduler_on_new_options
1 3 3 ./src/core/or/scheduler_kist.c(109):
each_channel_write_to_kernel
1 4 4 ./src/core/or/scheduler_kist.c(147):
socket_table_search
1 4 4 ./src/core/or/scheduler_kist.c(773):
scheduler_kist_set_lite_mode
1 4 4 ./src/core/or/scheduler_kist.c(783):
scheduler_kist_set_full_mode
1 5 5 ./src/core/or/scheduler_kist.c(118):
free_outbuf_info_by_ent
1 5 5 ./src/core/or/scheduler_kist.c(129):
free_socket_info_by_ent
1 5 5 ./src/core/or/scheduler_kist.c(430): MOCK_IMPL
1 5 5 ./src/core/or/scheduler_kist.c(441): have_work
1 8 5 ./src/core/or/scheduler_kist.c(95):
channel_outbuf_length
1 7 7 ./src/core/or/scheduler_kist.c(333):
outbuf_table_remove
1 7 7 ./src/core/or/scheduler_kist.c(346):
set_scheduler_run_interval
1 8 8 ./src/core/or/scheduler_kist.c(157):
free_socket_info_by_chan
1 10 8 ./src/core/or/scheduler_kist.c(396):
update_socket_written
1 12 8 ./src/core/or/scheduler_kist.c(794):
scheduler_can_use_kist
1 10 9 ./src/core/or/scheduler_kist.c(483):
kist_scheduler_init
1 14 9 ./src/core/or/scheduler_kist.c(359):
socket_can_write
1 10 10 ./src/core/or/scheduler_kist.c(301):
init_socket_info
1 10 10 ./src/core/or/scheduler_kist.c(318):
outbuf_table_add
1 11 11 ./src/core/or/scheduler_kist.c(379):
update_socket_info
1 16 11 ./src/core/or/scheduler_kist.c(751):
kist_scheduler_run_interval
2 31 20 ./src/core/or/scheduler_kist.c(507):
kist_scheduler_schedule
5 120 54 ./src/core/or/scheduler_kist.c(172): MOCK_IMPL
17 168 89 ./src/core/or/scheduler_kist.c(544):
kist_scheduler_run
Complexity Histogram
Score-Range Lin-Ct
0-9 218
************************************************************
10-19 89 ************************
Scored procedure ct: 26
Non-comment line ct: 307
Average line score: 6
25%-ile score: 1 (75% in higher score procs)
50%-ile score: 2 (half in higher score procs)
75%-ile score: 17 (25% in higher score procs)
Highest score: 17 (kist_scheduler_run() in
./src/core/or/scheduler_kist.c)
}}}
In the CLI I set the score threshold to 1 so that it ignores truly
trivial functions, but that can be set to 0 if wanted.
You can see the following:
- It tells us that there is a function with a bad nesting depth of 5.
- It tells us how big each function is (`nc-lns` counts only code).
- It tells us how big the file is (307 LoC).
- It tells us that the file contains 26 functions.
- It has a weird but configurable algorithm to calculate function score.
The only weird thing is that it miscalculates at which line a function is,
I think this has something to do with the way we comment our code but I
haven't figured it out yet (does not seem related to macros). And it also
does not work really well with `MOCK_IMPL` etc.
I can also pass it all the `.c` files of Tor and it will give me a project
wide summary which I have attached to this ticket.
Does this seem like a plausible way forward? Like, having a script tha
runs this in a useful way and parses the output to give some controlled
information? Or do we car eabout other best-practices from #29219 more?
This likely seems to be the lowest hanging fruit, but I'm open to more.
Also what do we want to do? Throw error if we have a nesting score above 7
(example)? Or just give out information?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/29221#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list