WIP: Prevent race condition around strerror which is not thread safe

[ushachar]

The compile time checks might need some additional tuning - verified on Linux & OSX but not beyond that.

[ushachar]

@oranagra an alternative approach to (#3) as @yossigo suggested.
I'm not at all sure this won't break on some OSes, but I did verify Linux & OSX

biggest pains in this approach are:

• Esoteric build flags
• The inability to place the error string anywhere other then the tail of the debug line

If you think this is a better approach, let me know and I'll work on getting it ready....

[yossigo]

@ushachar The simplest approach which would just work on any recent glibc system would be to just use %m formatting. This will also placing the error string anywhere in the text.

More esoteric platforms (OSX?) could have a compatibility function that handles this manually.

[ushachar]

@ushachar The simplest approach which would just work on any recent glibc system would be to just use %m formatting. This will also placing the error string anywhere in the text.

More esoteric platforms (OSX?) could have a compatibility function that handles this manually.

@yossigo At least according to the man page, %m uses strerror (not strerror_r)
Also:

• that would mean the usage of a macro all over, not just in that single function (unless you mean inside serverLogSysError -- in which case I think it's really not worth the added build time checks to toggle).
• %m uses the current errno, which means you cannot reliably print an error some lines after the issue

[yossigo]

@ushachar seems like the manpage might not be accurate and strerror_r(0 is used:
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/vfprintf-internal.c;h=e01c319b19ab1948f34d71f5d12f874cfd2d1cd9;hb=refs/heads/master#l1092

[oranagra]

i'm having hard time to tell which approach i prefer.

The one in #3 seems easier to use in all the places that log an errno, but the solution is more complicated architecture (e.g. freeErrorStrings).
The one in #4 has easier architecture (all in one function), but uglier in all the places that use it.

i.e the usage optins are these:
original:
serverLog(LL_WARNING,"Fatal error loading the DB: %s. Exiting.",strerror(errno));
previous PR:
serverLog(LL_WARNING,"Fatal error loading the DB: %s. Exiting.",redisError(errno));
This PR:
serverLogSysError(LL_WARNING, errno, "Fatal error loading the DB. Exiting.");

the %m thing seems unusable for us, both maybe the platform compatibility issues, but more importantly the fact it uses the current errno and not the provided one.

i'm leaning towards #3 being better because it's nicer in all the places that use it.

[yossigo]

@oranagra why? if we leave aside the implementation (i.e. we do that ourselves and don't depend on glibc), how about:

serverLogError(LL_WARNING, errno, "Fatal error loading DB: %m, exiting.");

[oranagra]

@yossigo that's similar in my eyes to the one we already have in this PR.
(which is what i said doesn't look so good in the place that calls it).
the difference is that you're suggesting that the error doesn't have to go at the end.

but keep in mind that you'll have to mess with the format string, some some prints can look like this:

serverLogError(LL_WARNING, errno, "Fatal db %d error: %m, exiting %s.", dbid, "now");

if we bother that much to mess with the format string, maybe we can also move the errno from being first argument and just use:

serverLog(LL_WARNING, "Fatal db %d error: %m, exiting %s.", dbid, errno, "now");

i.e. modify that normal serverLog function.

[ushachar]

The amount of code we'll need to write & maintain to re-implement a glibc independent %m is substantially larger (and more complex) then the code needed for #3 -- do you think it's worth it?

[ushachar]

@oranagra / @yossigo -- I see three options:

1. PR WIP: Prevent race condition around strerror which is not thread safe #3 (static allocation of error strings on startup) - somewhat hackish, very slight impact on start up time
2. PR WIP: Prevent race condition around strerror which is not thread safe #4 (This PR) - Build time macros might break on some OSes, limited to add the error string only at the end
3. PR # ? (Reimplement printf %m handling) - Build time macros might break on some OSes, much larger code to write / maintain

I tend to favor option 1, but 2 also seems very reasonable.
Seems to me option 3 has a low ROI given the amount of code which we'll need to maintain for it.

[oranagra]

i also prefer option 1 (#3). keeps the messy code in one place, and leaves the rest of the project relatively similar to how it was (compared to this PR).

[ushachar]

@yossigo - strong objection on your side for option 1?

[yossigo]

@ushachar I don't like option 1, a bit too much of a code smell.