fix extra warnings when building with -Wwrite-strings

[elliefm]

This PR fixes all the new warnings that appear when compiling with the -Wwrite-strings C dialect option (in which string literals have type const char[] rather than char[] that will crash if you modify it). This option is nice because it means the compiler's usual discarded-qualifiers warnings will now also protect us from doing dodgy stuff with string literals (unless we silence the warning with an explicit cast).

It also adds this option to CFLAGS in tools/build-with-cyruslibs.sh, which means once this is merged, all our Github CI runs will be built with this option, and will yell at us if we do dodgy stuff without at least a cast to shut it up.

There were lots of very similar fixes, which I've grouped up approximately one commit per type of fix. I suggest reviewing it commit by commit. You might also want to add -Wwrite-strings to your own local build options after this lands.

[dilyanpalauzov]

Why not changing:

--- a/imap/http_dav.h
+++ b/imap/http_dav.h
@@ -324,7 +324,7 @@ struct prop_entry {
     int (*put)(xmlNodePtr prop,         /* Callback to write property */
                unsigned set, struct proppatch_ctx *pctx,
                struct propstat *propstat, void *rock);
-    void *rock;                         /* Add'l data to pass to callback */
+    const void *rock;                   /* Add'l data to pass to callback */
 };
 
 /* Bitmask of property flags */

and not modifying http_caldav.c?

[elliefm]

Why not changing:

--- a/imap/http_dav.h
+++ b/imap/http_dav.h
@@ -324,7 +324,7 @@ struct prop_entry {
     int (*put)(xmlNodePtr prop,         /* Callback to write property */
                unsigned set, struct proppatch_ctx *pctx,
                struct propstat *propstat, void *rock);
-    void *rock;                         /* Add'l data to pass to callback */
+    const void *rock;                   /* Add'l data to pass to callback */
 };
 
 /* Bitmask of property flags */

and not modifying http_caldav.c?

Because it's not always const for all callbacks that use it. e.g. for "supported-method-set", this field is a pointer to another callback function.

[rsto]

will now also protect us from doing dodgy stuff with string literals (unless we silence the warning with an explicit cast).

I see that the PR does explicitly cast away the const from some string literals. But doesn't this indicate that either the API expecting a non-const is broken, or passing the string literal is wrong/dangerous?

[elliefm]

will now also protect us from doing dodgy stuff with string literals (unless we silence the warning with an explicit cast).

I see that the PR does explicitly cast away the const from some string literals. But doesn't this indicate that either the API expecting a non-const is broken, or passing the string literal is wrong/dangerous?

Yep. In the cases where I just shut it up with a cast, it was because it didn't seem worth the effort (right now) to fix the API or implementation in question. We can be reasonably assured that these specific cases are safe, since we aren't seeing the kind of crashes that would occur if they weren't. A lot of this this stuff isn't well tested, so even though I could potentially do some refactoring to handle the strings more sanely, it would be hard to prove that my refactoring hadn't introduced other bugs. Shutting up the warnings in these cases will let us collectively enable -Wwrite-strings sooner, and -Wwrite-strings will prevent us creating similar problems in new code and APIs, where we won't already know from experience that it's safe.

But I do think it would be good to fix those APIs and implementations properly! Ideally, we'd never ever need to cast away const from a string literal. But on balance, I think getting to the point where we can always use -Wwrite-strings is the higher priority. And that will help us get it right when we do eventually get around to fixing the other bits.

[dilyanpalauzov]

When I asked here why the interfaces are not changed to require const, the reply was that this will contradict program logic. When Robert raised the same question, the answer was different — this is not a priority.

[rsto]

Agreed. LGTM.

[elliefm]

When I asked here why the interfaces are not changed to require const, the reply was that this will contradict program logic. When Robert raised the same question, the answer was different — this is not a priority.

I explains the casts you each asked about; they were different casts, and had different explanations.