| Message ID | CABmJbFWC7ztmWHk87-j8Wn8VbPN3S--UdOXAfTNv7bsKVi1kyA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Holger, It seems there are no more comments on this code. Could you please merge it? It is independent of other changes. On Thu, Mar 13, 2014 at 12:37 PM, Alexander Chemeris <alexander.chemeris@gmail.com> wrote: > On Wed, Mar 12, 2014 at 10:53 PM, Daniel Willmann <dwillmann@sysmocom.de> wrote: >> Hello Alexander, >> >> On Fri, 2014-03-07 at 21:25, Alexander Chemeris wrote: >>> @@ -100,11 +100,13 @@ void gsm340_gen_scts(uint8_t *scts, time_t time) >>> time_t gsm340_scts(uint8_t *scts) >>> { >>> struct tm tm; >>> - uint8_t yr = gsm411_unbcdify(*scts++); >>> - int ofs; >>> + uint8_t yr, tz; >>> + int ofs = 0; >> >> Do we need to initialize ofs? It looks like both before and now ofs is >> always assigned before use. > > Yeah, a leftover from an intermediate implementation I had. Fixed. > >>> @@ -114,15 +116,28 @@ time_t gsm340_scts(uint8_t *scts) >>> tm.tm_hour = gsm411_unbcdify(*scts++); >>> tm.tm_min = gsm411_unbcdify(*scts++); >>> tm.tm_sec = gsm411_unbcdify(*scts++); >>> -#ifdef HAVE_TM_GMTOFF_IN_TM >>> - tm.tm_gmtoff = gsm411_unbcdify(*scts++) * 15*60; >>> -#endif >> >> By deleting the #ifdef here and not adding it below you'll break cygwin >> builds. See commit 7c8e2cc7aca1a789bbcf989a14be177d59041959. > > Fixed. > >>> /* according to gsm 03.40 time zone is >>> "expressed in quarters of an hour" */ >>> - ofs = gsm411_unbcdify(*scts++) * 15*60; >>> - >>> - return mktime(&tm) - ofs; >>> + tz = *scts++; >>> + ofs = gsm411_unbcdify(tz&0x7f) * 15*60; >>> + if (tz&0x80) >>> + ofs = -ofs; >>> + /* mktime() doesn't tm.tm_gmtoff into account. Instead, it fills this field >> doesn't take > > Thanks. > >>> + * with the current timezone. Which means that the resulting time is off >>> + * by several hours after that. So here we're setting tm.tm_isdt to -1 >>> + * to indicate that the tm time is local, but later we subtract the >>> + * offset introduced by mktime. */ >>> + tm.tm_isdst = -1; >>> + >>> + timestamp = mktime(&tm); >>> + if (timestamp < 0) >>> + return -1; >>> + >>> + /* Subtract artificial timezone offset, introduced by mktime() */ >>> + timestamp = timestamp - ofs + tm.tm_gmtoff; >>> + >>> + return timestamp; >>> } >>> >>> /* Decode validity period format 'relative'. >>> -- >>> 1.8.4.2 >> >> I'm still not convinced that we want to use mktime and tm_gmtoff for the >> calculation. There's timegm() which is the inverse of gmtime() which I >> think we should use were it's available. Depending on timegm shouldn't >> make us and more dependent on some extension than we already are with >> tm_gmtoff. >> >> The man page has a note on how to implement a portable version, though >> it involves calling setenv (to set TZ to UTC) and I'm not sure we want >> to do that in a library. >> >> Additional discussion welcome. > > I spent some time thinking about the best solution and came to > conclusion that the one with mktime() is the best one. > > gmtime() is a non-standard extension, so we'll have to detect it, add > #ifdef's and then we still will need a code which works if it is not > available. And that code will be based on mktime(). So why not to use > mktime() from the very beginning? > > The code is covered with tests, so we will notice any breakage if it > occurs on other platforms. > > -- > Regards, > Alexander Chemeris. > CEO, Fairwaves, Inc. / ООО УмРадио > https://fairwaves.co
From d3232c4c066497fa25365dd677069e24192e811a Mon Sep 17 00:00:00 2001 From: Alexander Chemeris <Alexander.Chemeris@gmail.com> Date: Fri, 7 Mar 2014 21:03:44 +0100 Subject: [PATCH 6/6] sms: Fix gsm340_scts() to correctly decode absolute valid times. - Support negative timezone offsets decoding. - Correctly account timezone offset and artificial offset mktime() introduces. --- src/gsm/gsm0411_utils.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index bb59a10..197f2c3 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -100,11 +100,13 @@ void gsm340_gen_scts(uint8_t *scts, time_t time) time_t gsm340_scts(uint8_t *scts) { struct tm tm; - uint8_t yr = gsm411_unbcdify(*scts++); + uint8_t yr, tz; int ofs; + time_t timestamp; memset(&tm, 0x00, sizeof(struct tm)); + yr = gsm411_unbcdify(*scts++); if (yr <= 80) tm.tm_year = 100 + yr; else @@ -114,15 +116,32 @@ time_t gsm340_scts(uint8_t *scts) tm.tm_hour = gsm411_unbcdify(*scts++); tm.tm_min = gsm411_unbcdify(*scts++); tm.tm_sec = gsm411_unbcdify(*scts++); -#ifdef HAVE_TM_GMTOFF_IN_TM - tm.tm_gmtoff = gsm411_unbcdify(*scts++) * 15*60; -#endif /* according to gsm 03.40 time zone is "expressed in quarters of an hour" */ - ofs = gsm411_unbcdify(*scts++) * 15*60; + tz = *scts++; + ofs = gsm411_unbcdify(tz&0x7f) * 15*60; + if (tz&0x80) + ofs = -ofs; + /* mktime() doesn't take tm.tm_gmtoff into account. Instead, it fills this + * field with the current timezone. Which means that the resulting time is + * off by several hours after that. So here we're setting tm.tm_isdt to -1 + * to indicate that the tm time is local, but later we subtract the + * offset introduced by mktime. */ + tm.tm_isdst = -1; + + timestamp = mktime(&tm); + if (timestamp < 0) + return -1; + + /* Take into account timezone offset */ + timestamp -= ofs; +#ifdef HAVE_TM_GMTOFF_IN_TM + /* Remove an artificial timezone offset, introduced by mktime() */ + timestamp += tm.tm_gmtoff; +#endif - return mktime(&tm) - ofs; + return timestamp; } /* Decode validity period format 'relative'. -- 1.7.9.5