Improve const correctness in coturn #1424

jonesmz · 2024-02-01T22:23:11Z

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.

jonesmz · 2024-02-01T22:23:45Z

.clang-tidy

@@ -12,5 +12,6 @@ Checks: 'clang-diagnostic-*,
 	,-readability-else-after-return,
 	,-readability-magic-numbers,
 	,-readability-function-cognitive-complexity,
+	,-readability-suspicious-call-argument,
 	,modernize-*,


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

@@ -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);


src/apps/relay/dtls_listener.c

@@ -232,7 +232,7 @@
    return NULL;
  }

-  int rc = ssl_read(sock->fd, ssl, nbh, server->verbose);
+  int const rc = ssl_read(sock->fd, ssl, nbh, server->verbose);


eakraly · 2024-02-04T23:58:33Z

While the change is great the syntax is not. I know both const int and int const are the same but former is more common (tbh honest I've never seen any code using int const)

I suggest we use the more common pattern const type

jonesmz · 2024-02-05T17:10:53Z

While the change is great the syntax is not. I know both const int and int const are the same but former is more common (tbh honest I've never seen any code using int const)

I suggest we use the more common pattern const type

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.

eakraly · 2024-02-11T06:01:27Z

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.
This is yet another silly flame war. Waste of time imho.

Thank you for sticking with code convention of this repo.

eakraly · 2024-02-11T06:04:42Z

src/apps/relay/http_server.c

@@ -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);


type const => const type

eakraly · 2024-02-11T06:06:48Z

src/apps/relay/ns_ioalib_engine_impl.c

@@ -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);


eakraly · 2024-02-11T06:06:58Z

src/apps/relay/ns_ioalib_engine_impl.c

@@ -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);


eakraly · 2024-02-11T06:08:16Z

src/apps/relay/turn_admin_server.c

  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);


clang-format you can do better

eakraly · 2024-02-11T06:08:50Z

src/apps/relay/turn_admin_server.c

@@ -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);


eakraly · 2024-02-11T06:12:21Z

src/apps/uclient/startuclient.c

@@ -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;


This is nice. clang-format did that?
Hope no one complains because of older compilers

eakraly · 2024-02-11T06:15:45Z

src/apps/uclient/startuclient.c

          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);


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?

@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.

eakraly · 2024-02-11T06:16:04Z

src/apps/uclient/startuclient.c

          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);


as above
there are more instance below - I will stop repeating my comment

eakraly · 2024-02-11T06:18:08Z

src/apps/uclient/startuclient.c

-
-    int len = send_buffer(clnet_info, &request_message, 1, atc);
-
+  while (1) {


eakraly · 2024-02-11T06:21:12Z

src/server/ns_turn_server.c

@@ -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);


Should find/replace all those SOCKET_TYPE const probably

eakraly · 2024-02-11T06:22:25Z

src/server/ns_turn_server.c

-  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);


Commenting so it is easier to find

ggarber · 2024-04-19T15:15:47Z

@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

jonesmz · 2024-04-19T20:17:47Z

@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.

jonesmz · 2024-05-16T15:45:37Z

@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.

…modified

…nction variables

jonesmz commented Feb 1, 2024

View reviewed changes

github-advanced-security bot found potential problems Feb 1, 2024

View reviewed changes

eakraly reviewed Feb 11, 2024

View reviewed changes

eakraly requested changes Feb 11, 2024

View reviewed changes

jonesmz and others added 4 commits May 22, 2024 15:59

Apply clang-tidy auto-fix-it to add const to variables which are not …

12e5bf7

…modified

Manually apply some const correctness, and reduce scope of various fu…

111a2a1

…nction variables

Adjust clang-format to enforce const-west

9883522

changed east-const to west-const where missed by clang-format

5842b7f

redraincatching force-pushed the const-correctness branch from cc4d4e8 to 5842b7f Compare May 22, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve const correctness in coturn #1424

Improve const correctness in coturn #1424

jonesmz commented Feb 1, 2024

jonesmz Feb 1, 2024

eakraly commented Feb 4, 2024

jonesmz commented Feb 5, 2024

eakraly commented Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

redraincatching May 17, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

eakraly Feb 11, 2024

ggarber commented Apr 19, 2024

jonesmz commented Apr 19, 2024

jonesmz commented May 16, 2024


		int len = send_buffer(clnet_info, &request_message, 1, atc);

		while (1) {

Improve const correctness in coturn #1424

Are you sure you want to change the base?

Improve const correctness in coturn #1424

Conversation

jonesmz commented Feb 1, 2024

Choose a reason for hiding this comment

eakraly commented Feb 4, 2024

jonesmz commented Feb 5, 2024

eakraly commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggarber commented Apr 19, 2024

jonesmz commented Apr 19, 2024

jonesmz commented May 16, 2024