-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve const correctness in coturn #1424
base: master
Are you sure you want to change the base?
Conversation
@@ -12,5 +12,6 @@ Checks: 'clang-diagnostic-*, | |||
,-readability-else-after-return, | |||
,-readability-magic-numbers, | |||
,-readability-function-cognitive-complexity, | |||
,-readability-suspicious-call-argument, | |||
,modernize-*, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag triggers a lot when functions have parameters that are just a bunch of ints. For a c-language codebase, it's basically useless.
src/apps/relay/mainrelay.c
Outdated
@@ -1908,7 +1908,7 @@ | |||
turn_params.no_tlsv1_2 = get_bool_value(value); | |||
break; | |||
case NE_TYPE_OPT: { | |||
int ne = atoi(value); | |||
int const ne = atoi(value); |
Check warning
Code scanning / PREfast
'value' could be '0'.
While the change is great the syntax is not. I know both I suggest we use the more common pattern |
east-const is the default behavior of clang-tidy fixits, and clang-format, and in wide-spread use in codebases in the wild. It's also substantially less complex than west-const in terms of cognitive load, as c-types are semantically read right-to-left. But if you insist, i can add a clang-format rule to do this. |
I have not seen any code using this convention. TBH just learned about it. Thank you for sticking with code convention of this repo. |
src/apps/relay/http_server.c
Outdated
@@ -54,7 +54,7 @@ struct http_headers { | |||
|
|||
static void write_http_echo(ioa_socket_handle s) { | |||
if (s && !ioa_socket_tobeclosed(s)) { | |||
SOCKET_APP_TYPE sat = get_ioa_socket_app_type(s); | |||
SOCKET_APP_TYPE const sat = get_ioa_socket_app_type(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type const => const type
@@ -2439,7 +2439,7 @@ static int socket_input_worker(ioa_socket_handle s) { | |||
if (s->st == TENTATIVE_TCP_SOCKET) { | |||
EVENT_DEL(s->read_event); | |||
#if TLS_SUPPORTED | |||
TURN_TLS_TYPE tls_type = check_tentative_tls(s->fd); | |||
TURN_TLS_TYPE const tls_type = check_tentative_tls(s->fd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
east west
@@ -2478,7 +2478,7 @@ static int socket_input_worker(ioa_socket_handle s) { | |||
} else if (s->st == TENTATIVE_SCTP_SOCKET) { | |||
EVENT_DEL(s->read_event); | |||
#if TLS_SUPPORTED | |||
TURN_TLS_TYPE tls_type = check_tentative_tls(s->fd); | |||
TURN_TLS_TYPE const tls_type = check_tentative_tls(s->fd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
if (buffer_size >= UDP_STUN_BUFFER_SIZE) { | ||
TURN_LOG_FUNC(TURN_LOG_LEVEL_WARNING, "%s: request is too big: %d\n", __FUNCTION__, buffer_size); | ||
to_be_closed = 1; | ||
} else if (buffer_size > 0) { | ||
|
||
SOCKET_TYPE st = get_ioa_socket_type(s); | ||
SOCKET_TYPE const st = get_ioa_socket_type(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format you can do better
src/apps/relay/turn_admin_server.c
Outdated
@@ -3352,7 +3352,7 @@ static void handle_https(ioa_socket_handle s, ioa_network_buffer_handle nbh) { | |||
} else { | |||
TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "%s: HTTPS request, path %s\n", __FUNCTION__, hr->path); | |||
|
|||
AS_FORM form = get_form(hr->path); | |||
AS_FORM const form = get_form(hr->path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and again
src/apps/uclient/startuclient.c
Outdated
@@ -473,11 +475,10 @@ static int clnet_allocate(int verbose, app_ur_conn_info *clnet_info, ioa_addr *r | |||
} | |||
{ | |||
int found = 0; | |||
for (stun_attr_ref sar = stun_attr_get_first(&response_message); sar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice. clang-format did that?
Hope no one complains because of older compilers
uint16_t chni = 0; | ||
int port = (unsigned short)turn_random(); | ||
if (port < 1024) { | ||
port += 1024; | ||
} | ||
addr_set_port(&arbaddr, port); | ||
uint8_t *u = (uint8_t *)&(arbaddr.s4.sin_addr); | ||
uint8_t *const u = (uint8_t *)&(arbaddr.s4.sin_addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my brain refuses to compile the next line...
pointer is const - I get it. But writing to u
defeats the purpose
Yes, content of u is not const
But still - what does const add here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eakraly - i've followed up on all of your comments on behalf of @jonesmz, except this one, and i'll explain why.
adding const to the pointer does two things:
- any errors that arise from attempting to mutate the pointer, however unlikely, are found at compile time, rather than through debugging
- it's more semantically correct, so a programmer who's newer to the codebase (such as myself) doesn't need to read through all the code to know that the pointer shouldn't change, rather the code itself tells them
if this is still an issue, let me know, and i will remove it.
if (i > 0) { | ||
addr_cpy(&arbaddr[i], &arbaddr[0]); | ||
} | ||
addr_set_port(&arbaddr[i], (unsigned short)turn_random()); | ||
uint8_t *u = (uint8_t *)&(arbaddr[i].s4.sin_addr); | ||
uint8_t *const u = (uint8_t *)&(arbaddr[i].s4.sin_addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
there are more instance below - I will stop repeating my comment
src/apps/uclient/startuclient.c
Outdated
|
||
int len = send_buffer(clnet_info, &request_message, 1, atc); | ||
|
||
while (1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
src/server/ns_turn_server.c
Outdated
@@ -1096,7 +1096,7 @@ static int handle_turn_allocate(turn_turnserver *server, ts_ur_super_session *ss | |||
*err_code = 442; | |||
*reason = (const uint8_t *)"UDP Transport is not allowed by the TURN Server configuration"; | |||
} else if (ss->client_socket) { | |||
SOCKET_TYPE cst = get_ioa_socket_type(ss->client_socket); | |||
SOCKET_TYPE const cst = get_ioa_socket_type(ss->client_socket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should find/replace all those SOCKET_TYPE const
probably
src/server/ns_turn_server.c
Outdated
int is_padding_mandatory = is_stream_socket(st); | ||
const size_t orig_blen = blen; | ||
SOCKET_TYPE const st = get_ioa_socket_type(ss->client_socket); | ||
SOCKET_APP_TYPE const sat = get_ioa_socket_app_type(ss->client_socket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting so it is easier to find
@jonesmz This is great and it is almost complete, good job! Can you take a look at the remaining comments from eakraly and we merge it ? Thank you very much |
Yes, give me a few weeks to work on it, been surprisingly busy. |
At the moment, it looks like i'll need to delegate finishing this work to one of my teammates. Hopefully they have an opportunity to address the remaining feedback soon. |
5842b7f
to
37325c5
Compare
37325c5
to
f7b80e4
Compare
Marking variables as const when they won't be modified after initialization helps programmers trying to understand a codebase to manage the cognative load.
This pull request uses a clang-tidy fixit (Hard to automate, since the code needs to be temporarily compiled as C++ for it to work) to try to mechanically apply the const keyword to code where the automated tool can determine that the variable won't be modified.
I then follow this up with a manual improvement pass to turnutils_uclient, where I address const correctness of local variables, as well as do some adjustments to loops and scoping to help with reducing complexity.