Opened 3 years ago

Closed 3 years ago

#25432 closed enhancement (implemented)

remove router.c internal functions from router.h

Reported by: valentecaio Owned by: valentecaio
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: easy refactor
Cc: caio-valente@… Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:


These functions (group A):

const char *router_get_description(char *buf, const routerinfo_t *ri);
const char *node_get_description(char *buf, const node_t *node);
const char *routerstatus_get_description(char *buf, const routerstatus_t *rs);
const char *extend_info_get_description(char *buf, const extend_info_t *ei);

Do the same as these (group B):

const char *router_describe(const routerinfo_t *ri);
const char *node_describe(const node_t *node);
const char *routerstatus_describe(const routerstatus_t *ri);
const char *extend_info_describe(const extend_info_t *ei);

With the difference that those last allocate a buffer, instead of forcing the caller to create and pass the buffer as a parameter.

The functions from group B are an abstraction to the ones from group A: they are better because they always generate buffers of enough size (the size is NODE_DESC_BUF_LEN). So, we should avoid using group A.

By now, both groups are declared in the header, and there is only one use of a function of group A (router_get_description is used on dirserv.c).

Also, all those functions are abstractions to format_node_description, that should also not be used externally, so we could also remove this one from the header.

The constant NODE_DESC_BUF_LEN is not necessary externally either.

Child Tickets

Change History (11)

comment:1 Changed 3 years ago by valentecaio

Cc: caio-valente@… added
Type: defectenhancement

comment:2 Changed 3 years ago by teor

Component: Core TorCore Tor/Tor
Milestone: Tor: 0.3.4.x-final
Version: Tor: unspecified

comment:3 Changed 3 years ago by valentecaio

Since each group A function would only be used once, by the respective group B function, I suggest that we suppress the group A.

So, I would do the following:

1- remove group A functions from header file.
2- merge group A functions into group B functions (remove declarations of group A functions).
3- remove format_node_description() declaration from header file.
4- move NODE_DESC_BUF_LEN constant from header to source file.
5- replace the external call (there is only one) to router_get_description() by a call to router_describe().

What do you think about this?

comment:4 Changed 3 years ago by nickm

That plan seems reasonable to me, if the situation is the way you describe it.

comment:5 Changed 3 years ago by valentecaio


It's done. Please check it on this branch:

comment:6 Changed 3 years ago by valentecaio

Status: assignedneeds_review

comment:7 Changed 3 years ago by dgoulet

Reviewer: asn

comment:8 Changed 3 years ago by asn

I plan to review this tomorrow. Thanks for the code! :)

comment:9 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

LGTM! I pushed branch t-25432 in my github repo ( with some basic fixes on the changefile, but other than that it's good!

Just as a future suggestion, valentecaio, I would ideally make multiple commits for such a refactoring project to make review easier. In particular, here is how I would structure my branch so that it's easier for the reviewer:

Commit A: Refactor the 4 groupA functions into groupB.
Commit B: Refactor directory_remove_invalid() to use router_describe().
Commit C: Misc improvements (move NODE_DESC_BUF_LEN, add docs, etc.)

All in all, this was a good refactoring! Thanks for the code! :)

comment:10 Changed 3 years ago by valentecaio

Thanks for the revision and the remarks, asn! I will mind this when working on future tickets.
Have a nice day.

comment:11 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

seems fine to me too. merging to master!

Note: See TracTickets for help on using tickets.