Fix all remaining compiler warnings #317
Conversation
|
Hi @svmhdvn! Thanks a lot for this contribution! Your patches look very nice, polished and straight forward. I'm a little bit time limited right now, but I definitely will take a closer look soon (next weekend?) and run some more tests on some legacy systems and compilers. |
There was a problem hiding this comment.
Most of this looks really great, and I'd like to definitely merge it! But so far I didn't run a single build or even test with this, so please give me some more time 😉
Probably you could have a look at my comments, especially the one regarding the compiler flags and GETID_LEN in client.c?
Ah, and I could definitely cherry-pick the "obvious" changes, like the one to .gitignore or regarding the version string? What do you think?
Alex
| @@ -723,7 +721,8 @@ AC_DEFINE_UNQUOTED(HOST_OS, "$host_os" ) | |||
|
|
|||
| # Add additional CFLAGS, LDFLAGS and LIBS which were specified on the command | |||
| # line or by some tests from above, but after running this script. Useful for | |||
| # adding "-Werror", for example: | |||
| # adding "-march=native", for example: | |||
| CFLAGS="$CFLAGS -pipe -Werror -Wall -Wextra -pedantic" | |||
There was a problem hiding this comment.
Without looking closer, I guess there are two problem with this:
- not all compilers support all of those flags (ngIRCd even supports pre-ANSI C compilers).
- some 3rd-party libraries (and their headers) can introduce warnings on some systems, therefore using
-Werrorby default is (sadly!) not an option.
There was a problem hiding this comment.
not all compilers support all of those flags (ngIRCd even supports pre-ANSI C compilers).
I know it supports pre-ANSI C compilers, but realistically, should the tiny minority of supported systems prevent enabling better warnings to catch more human errors (and possible security issues) for releases 28+? If a really old system really only had older compilers that didn't support these (relatively widespread) warning flags, then they can continue using release 27 since anyway, their system probably does not support newer dependencies like OpenSSL?
| @@ -38,7 +38,7 @@ | |||
| #include "match.h" | |||
| #include "messages.h" | |||
|
|
|||
| #define GETID_LEN (CLIENT_NICK_LEN-1) + 1 + (CLIENT_USER_LEN-1) + 1 + (CLIENT_HOST_LEN-1) + 1 | |||
| #define GETID_LEN (CLIENT_ID_LEN-1) + 1 + (CLIENT_USER_LEN-1) + 1 + (CLIENT_HOST_LEN-1) + 1 | |||
There was a problem hiding this comment.
Hm hm hm ... looking at this (new and old code), my brain goes in loops 🤯
Without digging much deeper, CLIENT_ID_LEN should be the maximum allowed length of a IRC "client ID", which in turn is <nick>!<user>@<host> for users.
So what is GETID_LEN, and why is it different to CLIENT_ID_LEN?
Adding the maximum user and host name length to CLIENT_ID_LEN looks even more bogus, because it should already include both ... no?
What am I missing? 🤔
There was a problem hiding this comment.
CLIENT_ID_LEN = 64 while CLIENT_NICK_LEN = 32. GETID_LEN is used as a size of the array here
Line 928 in 02a572d
Client->id. Going to the header file here Line 44 in 02a572d
CLIENT_ID_LEN, and the comment states that it could either hold a user nick or a server ID. Thus, my change should be correct to allow for the maximum size.
This fixes the (correctly caught) compiler warning.
| #if OPENSSL_VERSION_NUMBER < 0x10100000L | ||
| #define OpenSSL_version SSLeay_version | ||
| #define OPENSSL_VERSION SSLEAY_VERSION | ||
| #endif |
There was a problem hiding this comment.
I guess this will remove support for "legacy" OpenSSL versions? Do you know which? And if this is relevant for any currently supported (LTS) OS versions?
I mean, probably this is absolutely the right thing to do, but we should add a note to the "Upgrading" section in the INSTALL.md file if this could have an impact to supported systems?
Please just let me know if all this is nonsense, I'm not very familiar with the different OpenSSL versions at all ...
There was a problem hiding this comment.
I think we should add a note to the Upgrading section. Currently as per https://www.openssl.org/source (emphasis mine):
Note: The latest stable version is the 3.2 series supported until 23rd November 2025. Also available is the 3.1 series supported until 14th March 2025, and the 3.0 series which is a Long Term Support (LTS) version and is supported until 7th September 2026. All older versions (including 1.1.1, 1.1.0, 1.0.2, 1.0.0 and 0.9.8) are now out of support and should not be used. Users of these older versions are encouraged to upgrade to 3.2 or 3.0 as soon as possible.
Extended support for out of date openssl versions are only available by contract, so I don't think we should be specially supporting this in ngircd.
The current LTS is the 3.0 series, which is what the patch aims to support (and beyond).
| @@ -236,25 +236,40 @@ static bool | |||
| Load_DH_params(void) | |||
There was a problem hiding this comment.
Strictly speaking, I think there are more changes then really needed (like goto out instead if return false and if (fp == NULL)), and probably I would have moved OSSL_DECODER_CTX_new_for_pkey(...) below the fopen(...) to more closely follow the existing structure, but in the end I don't care much, all good, just my 5 cents, no changes needed, why not 😄
|
Any updates on this? I don't think there are any other changes to make right? |
This patch makes these changes:
Minor change while I was digging into the code for convenience:
All tests in the bundled testsuite are passing on both GCC and clang compilers.
Please test the DH param parsing deprecation fixes, as I don't use that feature and I'm not sure how to properly test it.