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.
- Why? Because code where conditionals lack braces is much harder to read, and prone to indentation confusion.
- How? Just added an extra flag to .clang-format and re-ran clang-format on all the files.
I also moved .clang-format up to the top level of the repo so that it can be applied to the fuzz targets as well.
I would like to get feedback on this and see if people is confortable
with these clang rules.
Right now is using the "llvm" style increasing the line length from 80
to 120 given that coturn is using long lines often.
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
The following changes have been made:
1. Replace deprecated functions with new standard functions
2. Add corresponding MSVC functions for non-standard functions
3. Remove warnings about unsafe functions
4. CMAKE: modify find pack Libevent and openssl
5. Modify include files
6. Use pthread4W
7. Modify socket in windows
8. Add CI - github action
8.1. msvc
8.2. mingw
10. The database:
9.1. sqlite, pgsql, hiredis, mongo is test compiled.
9.2. mysql, isnot test compiled.
11. The applications、server can be compiled and run successfully!
12. Add vcpkg manifest mode in cmake.
Store sanitized version of DB connection string with password masked
(replace all chars with * which exposes its length)
Use sanitized version when logging connection string
Fixes#1017 and #272
Using clang-tidy to detect unused header files
Inspired by #855
Test Plan:
- Rebuild all on mac, review no warnings/errors
- Pass builds/docker build - review for no issues
Ref https://www.sqlite.org/c3ref/initialize.html:
The sqlite3_initialize() interface is threadsafe, but
sqlite3_shutdown() is not.
We currently call `sqlite3_shutdown` from all threads as part of
`sqlite_init_multithreaded`, and this has been observed to have
adversarial effects during startup if many threads receive their first
inbound request at the same time. The apparent motivation behind
calling shutdown is to make the subsequent calls to `sqlite3_config`
succeed, since these ordinarily return SQLITE_MISUSE if called
multiple times. However, this function is also documented to not be
thread safe, so introduce a barrier that ensures we only initialize
once over all threads.
Because:
1. According to sqlite3 docs sqlite3_initialize() and sqlite3_shutdown() are not must to be invoked
2. sqlite3_initialize() is never called explicilty
3. sqlite3_shutdown() is not threadsafe and sqlite_init_multithreaded is not called holding a lock
4. According to docs all connections must be closed before invoking sqlite3_shutdown() but they are not (from the different threads).
Possible issue:
sqlite3_config must be called before sqlite3_initialize() or after sqlite3_shutdown() (and only once?)
Coturn does not disconnect database properly when shutdown.
This behavior causes accumulation of sockets for database with TIME_WAIT.
Especially, `turnadmin` is recommended to manage userdb,
but use of `turnadmin` from other application or scripts
can run out of socket resource due to above problem.
This patch adds database disconnection when shutdown
for both of `turnserver` and `turnadmin`.
Fixes#366
