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