Hello Willy,
Le 12/03/2016 07:54, Willy Tarreau a écrit :
>> +/* Return the GMT offset for a specific local time.
>> + * The string returned has the same format as returned by strftime(...
>> "%z", tm).
>> + * Offsets are kept in an internal cache for better performances.
>> + */
>> +const char *get_gmt_offset(struct tm *tm)
>> +{
>> + /* Cache offsets from GMT (depending on whether DST is active or not) */
>> + static char gmt_offsets[2][5+1] = { "", "" };
>> +
>> + int old_isdst = tm->tm_isdst;
>> + char *gmt_offset;
>> +
>> + /* Pretend DST not active if its status is unknown, or strftime() will
>> return an empty string for "%z" */
>> + if (tm->tm_isdst < 0) {
>> + tm->tm_isdst = 0;
>> + }
>> +
>> + /* Fetch the offset and initialize it if needed */
>> + gmt_offset = gmt_offsets[tm->tm_isdst & 0x01];
>> + if (unlikely(!*gmt_offset)) {
>> + strftime(gmt_offset, 5+1, "%z", tm);
>> + }
>> +
>> + /* Restore previous DST flag */
>> + tm->tm_isdst = old_isdst;
>> +
>> + return gmt_offset;
>> +}
> Since the cache is local to the function and initialized on the fly, no
> call is performed anymore on startup, it will fail on some chrooted
> systems because when the first log is emitted, you don't have access
> to the timezones anymore.
You just need to call tzset() once before chrooting to initialize the timezone
subsystem.
In fact, it's probably done by chance, there are a handful of functions that
call tzset implicitly.
I tested on a chroot environment on Linux/x86 and it works, but I admit it's
fragile.
I could cook an amended patch with an explicit call to tzset at init time.
> Thus I'd suggest to move the static gmt_offset out of the function
> and to have init_gmt_offset() which sets the string for both values
> of tm_isdst and which is called upon startup, and get_gmt_offset()
> which simply returns the correct one. At this point it will be so
> small that it's worth inlining it.
I obviously thought about a solution like that, but it's not that simple:
tm_isdst it not an isolated flag.
It cannot be interpreted without looking at a full struct tm.
tm_isdt can only be 0 only during the winter, and can be 1 only during the
summer.
For countries without a DST, the only valid value is 0.
Combining random dates with random values of tm_isdst has undefined behavior
and could lead to some portability problems.
In fact, there exists only one hour during which it can be 0 or 1: when going
from summer time to winter time for timezones that have a DST.
Think about it: there will exist two "Oct 30 2016, 02:30" in France, because
time will go backward once at 03:00.
This is the only time when both values of tm_isdst are valid at the same time,
and it's that value that allows to distinguish between those two equivalent
times.
That's why I chose to have a cache, so that I can use "real" times to compute
GMT offsets.
> An extra benefit of proceeding like this is that when we move to
> multi-threading, we won't have to lock for something which will only
> have to be initialized twice :-)
Wow, I didn't think haproxy would move to multi-threading one day.
I guess you'll have some local thread contexts (TLS or explicit context) where
the cache would fit nicely.
The cache would be initialized only once per thread and I guess you won't need
more threads than the number of CPU cores.
> Thanks!
> Willy
Let me know if you want a patch with an explicit call to tzset().
Benoît GARNIER
