Hi Benoit, On Sat, Mar 12, 2016 at 09:45:25AM +0100, Beno?t GARNIER wrote: > > 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.
Yes I'd appreciate it then. > > 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. OK I wasn't aware of this, that makes sense then. > 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. Yep definitely. The usual 90k seconds day :-) > 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. That makes sense, thanks for explaining. > > 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. Don't be too excited, it will take decades :-) > I guess you'll have some local thread contexts (TLS or explicit context) > where the cache would fit nicely. Possibly, yes. At least it won't be static in the function anymore. > The cache would be initialized only once per thread and I guess you won't > need more threads than the number of CPU cores. On this last point we don't know. Once we get some shared memory between all processes, it could make sense to have many threads to benefit from some slow external operations (HW crypto accelerators for example, which may require tens to hundreds of threads to be useful). > > Thanks! > > Willy > Let me know if you want a patch with an explicit call to tzset(). Yes, please do it and I'll merge it with the upcoming release. Thanks! Willy
