Aditya Garg <gargaditya08@xxxxxxxx> writes:
> Ok so I was wrong here. The warning actually comes when we compile
> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
> will attract waddress warnings,
But the patch in this thread, when compiled with NO_OPENSSL, gives
"-Wunreachable".
--- * ------ * ------ * ------ * ------ * ---
$ make V=1 CC=clang DEVELOPER=YesPlease CFLAGS=-Waddress NO_OPENSSL=NoThanks imap-send.o
* new build flags
clang -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP -Werror -Wall -pedantic -Wpedantic -std=gnu99 -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -Wwrite-strings -fno-common -Wunreachable-code -Wcomma -Wtautological-constant-out-of-range-compare -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Waddress -I. -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DNO_OPENSSL -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_SYSINFO -DHAVE_GETDELIM -DHAVE_GETRANDOM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' imap-send.c
imap-send.c:1344:5: error: code will never be executed [-Werror,-Wunreachable-code]
1344 | memset(&cb, 0, sizeof(cb));
| ^~~~~~
imap-send.c:1322:5: error: code will never be executed [-Werror,-Wunreachable-code]
1322 | memset(&cb, 0, sizeof(cb));
| ^~~~~~
imap-send.c:1300:5: error: code will never be executed [-Werror,-Wunreachable-code]
1300 | memset(&cb, 0, sizeof(cb));
| ^~~~~~
imap-send.c:1278:5: error: code will never be executed [-Werror,-Wunreachable-code]
1278 | memset(&cb, 0, sizeof(cb));
| ^~~~~~
4 errors generated.
make: *** [Makefile:2820: imap-send.o] Error 1
--- * ------ * ------ * ------ * ------ * ---
See the patch attached at the end [*] that you can apply to 'seen'
to try out; the result compiles with all combinations of gcc/clang
and with and without NO_OPENSSL.
Having said that, I think you should first clean up the repetitions
before piling a lot of different auth_method on top.
A part of the resulting code after applying [*] to 'seen' looks like
this:
} else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
if (!CAP(AUTH_CRAM_MD5)) {
fprintf(stderr, "You specified "
"CRAM-MD5 as authentication method, "
"but %s doesn't support it.\n", srvc->host);
goto bail;
}
/* CRAM-MD5 */
memset(&cb, 0, sizeof(cb));
cb.cont = auth_cram_md5;
if (NOT_CONSTANT(!cb.cont)) {
fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
"you have to build git-imap-send with OpenSSL library.");
goto bail;
}
if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
goto bail;
}
This pattern repeats for different auth methods as you add
copy-and-paste, which made the code way too long and too deeply
indented to be readable. Why not make the above into a single
helper function that you can reuse, so that the call site can become
something like
} else if (!strcmp(srvc->auth_method, "PLAIN")) {
if (try_auth_method(srvc, ctx, "PLAIN", AUTH_PLAIN, auth_plain))
goto bail;
} else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
if (try_auth_method(srvc, ctx, "CRAM-MD5", AUTH_CRAM_MD5, auth_cram_md5))
goto bail;
} ...
If you did it at the very beginning of the series (i.e. when you
only have CRAM_MD5 you inherited from the original before adding all
the other new ones), each addition would become a lot simpler and
main flow in the the resulting code would become quite simple like
the above.
static int try_auth_method(struct imap_server_conf *srvc,
struct imap_store *ctx,
const char *auth_method,
enum CAPABILITY cap,
int (*fn)(struct imap_store *, const char *))
{
struct imap_cmd_cb cb = {0};
if (!CAP(cap)) {
fprintf(stderr, "You specified "
"%s as authentication method, "
"but %s doesn't support it.\n",
auth_method, srvc->host);
return -1;
}
cb.cont = fn;
if (NOT_CONSTANT(!cb.cont)) {
fprintf(stderr, "If you want to use %s authentication mechanism, "
"you have to build git-imap-send with OpenSSL library.",
auth_method);
return -1;
}
if (imap_exec(ctx, &cb, "AUTHENTICATE %s", auth_method) != RESP_OK) {
fprintf(stderr, "IMAP error: AUTHENTICATE %s failed\n",
auth_method);
return -1;
}
return 0;
}
So here is that [*] patch that is incorrectly indented to avoid even
deeper nesting.
---- >8 ----
imap-send.c | 49 ++++++++++++++++++++++++-------------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git i/imap-send.c w/imap-send.c
index 6dbbf37125..bba2ef0cd0 100644
--- i/imap-send.c
+++ w/imap-send.c
@@ -1267,16 +1267,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
goto bail;
}
- #ifdef NO_OPENSSL
+ /* PLAIN */
+ memset(&cb, 0, sizeof(cb));
+ cb.cont = auth_plain;
+
+ if (NOT_CONSTANT(!cb.cont)) {
fprintf(stderr, "You are trying to use PLAIN authentication mechanism "
"with OpenSSL library, but its support has not been compiled in.");
goto bail;
- #endif
-
- /* PLAIN */
+ }
- memset(&cb, 0, sizeof(cb));
- cb.cont = auth_plain;
if (imap_exec(ctx, &cb, "AUTHENTICATE PLAIN") != RESP_OK) {
fprintf(stderr, "IMAP error: AUTHENTICATE PLAIN failed\n");
goto bail;
@@ -1289,16 +1289,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
goto bail;
}
- #ifdef NO_OPENSSL
- fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
- "you have to build git-imap-send with OpenSSL library.");
- goto bail;
- #endif
/* CRAM-MD5 */
-
memset(&cb, 0, sizeof(cb));
cb.cont = auth_cram_md5;
+
+ if (NOT_CONSTANT(!cb.cont)) {
+ fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
+ "you have to build git-imap-send with OpenSSL library.");
+ goto bail;
+ }
if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
goto bail;
@@ -1311,16 +1311,15 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
goto bail;
}
- #ifdef NO_OPENSSL
- fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
- "with OpenSSL library, but its support has not been compiled in.");
- goto bail;
- #endif
-
/* OAUTHBEARER */
-
memset(&cb, 0, sizeof(cb));
cb.cont = auth_oauthbearer;
+
+ if (NOT_CONSTANT(!cb.cont)) {
+ fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
+ "with OpenSSL library, but its support has not been compiled in.");
+ goto bail;
+ }
if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
goto bail;
@@ -1333,16 +1332,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
goto bail;
}
- #ifdef NO_OPENSSL
+ /* XOAUTH2 */
+ memset(&cb, 0, sizeof(cb));
+ cb.cont = auth_xoauth2;
+
+ if (NOT_CONSTANT(!cb.cont)) {
fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
"with OpenSSL library, but its support has not been compiled in.");
goto bail;
- #endif
-
- /* XOAUTH2 */
+ }
- memset(&cb, 0, sizeof(cb));
- cb.cont = auth_xoauth2;
if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
goto bail;
