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.

Sign up for GitHub

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

Jump to bottom

Address clang-tidy warnings in db files #1405

Open

jonesmz wants to merge 2 commits into coturn:master from jonesmz:clang-tidy-dbdrivers

Contributor

jonesmz commented Jan 25, 2024 •

edited

The general approach here was:

Always declare variables as close to where they are defined as possible.
Check for pre-conditions of functions before doing work (e.g. ensure we can connect to the DB before doing a bunch of string formatting)
Keep the scope of mutexes as reasonably small as practical.
Use idiomatic C11, such as for-loops over the thing being iterated, not while() loops over constants, or variables that aren't modified.
Prefer if(fail){return} function-body after over `if(not fail){ function-body inside if} return;

Clang-tidy returns a clean bill of health, but while going through this file i noticed a lot of things that raise questions.

Lack of checking column counts. Lack of handling the possibility of multiple return values. Questionably handling of strings. Complete lack of checking function inputs for invalid values (e.g. nullptr).

I'm not going to fix those, my organization doesn't USE the DB drivers, so i have little interest in re-working the logic beyond addressing clang-tidy warnings for my own sanity, but i did add TODO comments for someone else to look at in the future.

Additional note: While the changes look very invasive.... they aren't.

I don't think there is a way to get github to ignore whitespace in the filediff, but if someone were to compare the commit locally, they'll see that almost all of the changes are just adjusting indentation.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_mongo.c

    
                        } else {

                          ret = 0;

                        }

                        convert_string_key_to_binary(kval, key, sz / 2);

Contributor Author

jonesmz Jan 25, 2024

convert_string_key_to_binary() never returned anything but 0.
Changed the function to return void to simplify logic at call-sites.

jonesmz commented

View reviewed changes

src/ns_turn_defs.h

@@ @@ -184,6 +184,7 @@ typedef uint32_t turn_time_t; @@
               #error WRONG BYTE_ORDER SETTING
               #endif
+              // NOLINTBEGIN(clang-diagnostic-string-compare)

Contributor Author

jonesmz Jan 25, 2024

Suppresses clang-tidy warning about comparing the address of constant string literals is implementation specific behavior.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbdriver.c

    
            @@ -45,7 +45,7 @@ static void make_connection_key(void) { (void)pthread_key_create(&connection_key
          
              pthread_key_t connection_key;

              pthread_once_t connection_key_once = PTHREAD_ONCE_INIT;

              int convert_string_key_to_binary(char *keysource, hmackey_t key, size_t sz) {

              void convert_string_key_to_binary(char const *keysource, hmackey_t key, size_t sz) {

Contributor Author

jonesmz Jan 25, 2024

convert_string_key_to_binary() never returned anything but 0.
Changed the function to return void to simplify logic at call-sites.

Also, changed input parameter to char const* to allow the optimizer to assume that the contents of keysource won't be modified.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c

                 }
               #endif
-                return;

Contributor Author

jonesmz Jan 25, 2024

Eliminate clang-tidy warning about unnecessary return at end of void function.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c Outdated

               }
               static int donot_print_connection_success = 0;
               static void fix_user_directory(char *dir0) {
                 char *dir = dir0;
-                while (*dir == ' ')
+                while (*dir == ' ') {

Contributor Author

jonesmz Jan 25, 2024

all control flow should always have braces

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c

                     "CREATE TABLE admin_user (name varchar(32), realm varchar(127), password varchar(127), primary key (name))",
                     NULL};
-                int i = 0;
-                while (statements[i]) {
+                for (int i = 0; statements[i] != NULL; ++i) {

Contributor Author

jonesmz Jan 25, 2024

initialize variables as close to where they are defined as possible.

in this case, by using a for loop instead of while loop.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c

                   sqlite3_stmt *statement = NULL;
-                  int rc = 0;
-                  if ((rc = sqlite3_prepare(sqliteconnection, statements[i], -1, &statement, 0)) == SQLITE_OK) {
+                  if (sqlite3_prepare(sqliteconnection, statements[i], -1, &statement, 0) == SQLITE_OK) {

Contributor Author

jonesmz Jan 25, 2024

int rc is written to but never read from.

This pattern repeats throughout the whole file.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c

@@ @@ -212,752 +206,799 @@ static sqlite3 *get_sqlite_connection(void) { @@
                 persistent_users_db_t *pud = get_persistent_users_db();
                 sqlite3 *sqliteconnection = (sqlite3 *)pthread_getspecific(connection_key);
-                if (!sqliteconnection) {
+                if (sqliteconnection != NULL) {

Contributor Author

jonesmz Jan 25, 2024

Exit early if the connection is already available.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c

-                  sqlite_lock(0);
+                sqlite3 * const sqliteconnection = get_sqlite_connection();
+                if (sqliteconnection == NULL) {

Contributor Author

jonesmz Jan 25, 2024

exit early if the connection cant be established.

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c Outdated

-                        int type = sqlite3_column_type(st, 0);
-                        if (type != SQLITE_NULL)
-                          add_to_secrets_list(sl, (const char *)sqlite3_column_text(st, 0));
+                    for(int stepResult = sqlite3_step(st);

Contributor Author

jonesmz Jan 25, 2024

Use a for loop over the result of sqlite3_step() instead of a while-loop over the non-changing ctotal (the number of columns) variable.

While the control flow wasn't necessarily... "wrong" before, this is more idomatic.

jonesmz force-pushed the clang-tidy-dbdrivers branch from 3389943 to 2a0a9c3 Compare

January 25, 2024 23:54

jonesmz commented

View reviewed changes

src/apps/relay/dbdrivers/dbd_sqlite.c

-                        lock_realms();
-                        rp->options.perf_options.max_bps = turn_params.max_bps;
-                        unlock_realms();
+                    lock_realms();

Contributor Author

jonesmz Jan 26, 2024

This function was particularly gnarly.

Locking a mutex for a single variable, when there are 2 others to modify as well?

I don't fully know what this function is actually trying to accomplish, but it's a pretty good one for an expert to review.

Collaborator

eakraly commented Jan 28, 2024

@jonesmz pls rebase on latest master to fix formatting issues

eakraly changed the title ~~Address clang-tidy warnings in src/apps/relay/dbdrivers/sqlite.c~~ Address clang-tidy warnings in db files

Collaborator

eakraly commented Jan 28, 2024

I took the liberty to rename the PR to more properly reflect the extend of the change @jonesmz

Contributor Author

jonesmz commented Jan 28, 2024

@jonesmz pls rebase on latest master to fix formatting issues

Sure, will do on monday.

Contributor Author

jonesmz commented Jan 28, 2024

I took the liberty to rename the PR to more properly reflect the extend of the change @jonesmz

I did make modifications to each of the DB drivers because the function signature of that one function is being changed, but really the change is 99% for the sqlite driver.

Would it be better if i split the PR into two? One for sqlite, the other for the one function?

Collaborator

eakraly commented Jan 28, 2024

I took the liberty to rename the PR to more properly reflect the extend of the change @jonesmz

I did make modifications to each of the DB drivers because the function signature of that one function is being changed, but really the change is 99% for the sqlite driver.

Would it be better if i split the PR into two? One for sqlite, the other for the one function?

It is fine doing all of it in one PR actually. Makes more sense. Just updated the name.
Thank you!

jonesmz force-pushed the clang-tidy-dbdrivers branch from 2a0a9c3 to d57280e Compare

January 29, 2024 17:40

Contributor Author

jonesmz commented Jan 29, 2024

Most recent commit does a small amount of re-work for the sqlite_lock() and sqlite_unlock() functions to normalize platform specific code differences. No logic changes, just lifting ifdefs up to the global level instead of littering the functions with the differences.

jonesmz force-pushed the clang-tidy-dbdrivers branch from 746cd66 to 6e1def7 Compare

January 29, 2024 18:42

jonesmz added 2 commits

May 21, 2024 00:15


          Address clang-tidy warnings in src/apps/relay/dbdrivers/sqlite.c

e58f718


          Refactor sqlite database driver pthread handling to reduce difference…

fbab2d5

…s between platforms

jonesmz force-pushed the clang-tidy-dbdrivers branch from 6e1def7 to fbab2d5 Compare

May 21, 2024 05:15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment