Additionally improved Makefiles for compilation with Visual Studio 2015 where environmental variable "CPU" is not set.
Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-9839
(1) We seem to have a bit outdated code in SYSTEM_LOCALTIME(). In src/libs/zbxlog/log.c we have a modern get_time() which is supposed to work on different platforms and be thread-safe. Let's reuse it in SYSTEM_LOCALTIME(), it will simplify the code and make it more robust. Time zones is a complicated matter and it is good to have all related code in one place.
viktors.tjarve RESOLVED in r59124
get_time() moved to src/libs/zbxcommon/misc.c for better usability and SYSTEM_LOCALTIME() is using it now.
glebs.ivanovskis Looks good! Please have a look at minor fix in r59224.
viktors.tjarve Looks good. CLOSED
(2) Not directly related to compilation on Windows, so feel free to register this as a separate ZBX. 
The problem is with gmtoff. It is supposed to store the difference between local time and UTC. We are lucky if our system provides struct tm with tm_gmtoff which is adjusted to daylight saving time automatically. Otherwise we have to use timezone variable which does not take DST into account.
On my machine I get:
$ TZ='' src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-03-10,14:24:28.016,+00:00] <--- UTC $ TZ=tzn01 src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-03-10,13:24:38.360,-01:00] <--- local timezone $ TZ=tzn01dst02,0,365 src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-03-10,12:24:40.272,-02:00] <--- local timezone with DST applied
When I manually comment out #define HAVE_TM_TM_GMTOFF 1 and recompile:
$ TZ='' src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-03-10,14:27:08.160,+00:00] <--- UTC $ TZ=tzn01 src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-03-10,13:27:10.096,-01:00] <--- local timezone $ TZ=tzn01dst02,0,365 src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-03-10,12:27:12.456,-01:00] <--- local timezone with DST
Note that in the latter case with and without DST time differs but offset is the same.
Similar problem can be with VMware timestamps.
viktors.tjarve RESOLVED in r59124.
Issue with DST for vmware timestamps RESOLVED in r59128.
glebs.ivanovskis You pass NULL to get_time() which dereferences it, but never mind, I've split this subissue into (3) and (4). This one should be considered CLOSED
(3) For system.localtime[local] we need to calculate local time offset. If struct tm has tm_gmtoff we simply need to convert it to hours and minutes, otherwise the only portable solution I see is to call gmtime()_r (POSIX) or gmtime_s() (Windows) and calculate offset from two struct tm's. I'd encapsulate it into get_time(), let it always return struct zbx_tm with gmtoff field.
This way all time zone and daylight saving time issues will be out of our area of responsibility. Currently our assumption on one-hour DST does not always work. Take for example Antarctica/Troll and Australia/Lord_Howe time zones.
#define HAVE_TM_TM_GMTOFF 1
$ TZ=:UTC src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-04-01,10:41:54.647,+00:00] $ TZ=:Antarctica/Troll src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-04-01,12:41:57.904,+02:00] $ TZ=:Australia/Lord_Howe src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-04-01,21:42:00.895,+11:00]
#undef HAVE_TM_TM_GMTOFF
$ TZ=:UTC src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-04-01,10:42:15.553,+00:00] $ TZ=:Antarctica/Troll src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-04-01,12:42:17.399,+01:00] $ TZ=:Australia/Lord_Howe src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-04-01,21:42:20.377,+11:30]
viktors.tjarve RESOLVED in development branch svn://svn.zabbix.com/branches/dev/ZBX-9839 r59414
glebs.ivanovskis Several flaws:
- Does not compile (on Linux) because of misplaced #endif in get_time().
- Would be nice to bring declaration of tm_utc, day_diff and year_diff into if (NULL != tz_offset) block.
- If year_diff is not zero day_diff will be initialized twice.
- Time structure subtraction logic seems too complicated for me. Plus it's wrong. For example, for time zone UTC-00:30 the calculated offset will fluctuate. I believe there is a more concise, elegant and fool-proof solution.
- Passing struct timeval * instead of time_t * to time is incorrect. Let's go back to _ftime().
REOPENED
viktors.tjarve RESOLVED all issues the way suggested above.
glebs.ivanovskis I see no point in defining struct zbx_tz_offset fields as short, initializing them with (short)... and printing them using (int) casting. It's worth step back a little and rethink how we store offset. We try to keep minutes positive and store sign information in hours, this means that we can't store offsets like "-00:mm".
There is no sense in using "secure" Windows _s functions without checking return code. As discussed with sandis.neilands using THIS_SHOULD_NEVER_HAPPEN inside zbx_get_time() which is called inside log lock is dangerous and not necessary. zbx_get_time() can be greatly simplified.
viktors.tjarve RESOLVED
glebs.ivanovskis SYSTEM_LOCALTIME() looks good. Work on zbx_get_time() shall be continued in (7). Have a look at my suggestions in r59748 and all is OK close this subissue. Still RESOLVED
viktors.tjarve CLOSED
(4) VMware timestamps are considered to be in UTC. Unfortunately, there is no standard function to assemble timestamp from broken-down time structure. There are timegm() and mkgmtime() but no portable solution. I spoke to sasha and he agreed that it would be useful to have our own function for this purpose. Conversion should be straightforward since there is no DST in UTC. We can draw inspiration from here and here. The latter is probably overly complicated, it tries to check if "hh:mm:60" and "hh:mm:61" are valid leap seconds.
viktors.tjarve RESOLVED in development branch svn://svn.zabbix.com/branches/dev/ZBX-9839 r59414
Note that on a system which has 2 byte int value assigned to timestamp will be incorrect. 2 byte int can not hold value of Epoch time.
glebs.ivanovskis Things to improve here:
- zbx_mkgmtime() makes humble attempts to validate and normalize input (month and year). We should either remove all validation and leave timestamp calculation only or validate every field of input tm structure. I would prefer second option given the fact that input comes from outside.
- Lots of "magic numbers" (60, another 60, 24, 365, 70, 69, 299).
One more thing to discuss. We are not obliged to use struct tm return time_t unless we want to mimic mktime() as closely as possible. If we use explicit types we will have more control over ranges (as you rightly pointed out about int).
REOPENED
viktors.tjarve RESOLVED all issues the way suggested above.
glebs.ivanovskis Variable of type struct tm is no longer needed in vmware_get_events(). See my changes in r59531.
Parameter validation in zbx_utc_time() works incorrectly. (Fails in case of "23:mm:ss", for example.) Would be nice to sync parameter names in zbx_utc_time() declaration, definition and call(s).
Still RESOLVED
viktors.tjarve Changes look good. Also RESOLVED the imperfections.
glebs.ivanovskis Now vmware_get_events() looks good. Just a minor stylistic fix in r59748. Let's deal with zbx_utc_time() in (6). Feel free to close this subissue if you are happy about it. Still RESOLVED.
viktors.tjarve vmware_get_events() is good. CLOSED
(5) Since get_time() is not static any more it is a symbol with external linkage. It should be prefixed to avoid conflicts with libraries like ZBX-8629.
viktors.tjarve RESOLVED by adding a prefix zbx_.
glebs.ivanovskis CLOSED
(6) Input parameter validation in zbx_utc_time() is still incorrect. Also, might be a good idea to return SUCCEED or FAIL and store the conversion result in a separate [OUT] parameter.
viktors.tjarve RESOLVED
glebs.ivanovskis Calculation is off by one month. Expression for *t can be rewritten without new DAYS_PER_YEAR, HOURS_PER_DAY and MIN_PER_HOUR using only existing SEC_PER_YEAR, SEC_PER_DAY and SEC_PER_HOUR macros. I think it will reduce number of parentheses and make the expression easier to read and understand.
REOPENED
viktors.tjarve RESOLVED
glebs.ivanovskis Not exactly what I was hoping for. Have a look at r59975. Shortened the code a bit, added month day validation. Perhaps, we should borrow day_in_month() from timer.c? Still RESOLVED
viktors.tjarve Please take a look at the relocated day_in_month() r59995. RESOLVED
glebs.ivanovskis You corrected mon in the wrong way, fixed in r59998. Other than that, look fine! I've also noticed really rare bug (happens once in few centuries ) in timer, fixed in r60000. Relocated ZBX_LEAP_YEARS from config.h and made ZBX_IS_LEAP_YEAR into a static function (simply feels safer) in r60005. Still RESOLVED
viktors.tjarve Looks good. CLOSED
(7) In zbx_get_time() an assumption is made that time zone offset can't be more than 24 hours. It's not always true, theoretically one can set time zone with up to 25:59:59 offset:
$ TZ="" date && TZ=MTZ-24:59:59MST,0,365 date Wed Apr 27 12:14:36 UTC 2016 Thu Apr 28 14:14:35 MST 2016
We have this case covered most of the year, but offset will be incorrect on December 31 and January 1. Possible fix in pseudocode:
while (local.year > utc.year) local.yday += 365 + is_leap_year(--local.year); while (utc.year > local.year) utc.yday += 365 + is_leap_year(--utc.year); offset_min = ((local.yday - utc.yday) * 24 + local.hour - utc.hour) * 60 + local.min - utc.min;
viktors.tjarve RESOLVED
glebs.ivanovskis tz_offset->tz_sign not initialized in one #ifdef branch.
$ src/zabbix_agent/zabbix_agentd -t system.localtime[local] system.localtime[local] [s|2016-05-05,13:52:03.342,�03:00]
Also if we decided to use "not safe" Windows functions we should substitute gmtime_s() with gmtime() for consistency.
REOPENED
viktors.tjarve RESOLVED
glebs.ivanovskis We were modifying *tm that we were supposed to return to caller. Resolved this issue and restructured code a bit in r59975. Please have a look. Still RESOLVED
viktors.tjarve Looks good. CLOSED
Last thing I would suggest before the final merge is to group time- and calendar-related functions in src/zbxcommon/misc.c like they are in include/common.h:
double zbx_time(void); void zbx_timespec(zbx_timespec_t *ts); double zbx_current_time(void); void zbx_get_time(struct tm *tm, long *milliseconds, zbx_timezone_t *tz); int zbx_utc_time(int year, int mon, int mday, int hour, int min, int sec, int *t); int zbx_day_in_month(int year, int mon);
Successfully tested!
viktors.tjarve DONE with a small difference - the order of the old functions was not changed in misc.c file:
void zbx_timespec(zbx_timespec_t *ts); double zbx_time(void); double zbx_current_time(void); void zbx_get_time(struct tm *tm, long *milliseconds, zbx_timezone_t *tz); int zbx_utc_time(int year, int mon, int mday, int hour, int min, int sec, int *t); int zbx_day_in_month(int year, int mon);
Released in:
- pre-3.0.3rc1 r60049
- pre-3.1.0 (trunk) r60048