[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. -- Regards, Alexander Chemeris. CEO, Fairwaves, Inc. / ООО УмРадио https://fairwaves.co
Hello Alexander, On Fri, 2014-03-07 at 21:25, Alexander Chemeris wrote:
- Support negative timezone offsets decoding. - Correctly account timezone offset and artificial offset mktime() introduces.
patch seems missing, I included the current one:
From b8ed8ed29d239693e2b32b62cbac3b2847937b9b Mon Sep 17 00:00:00 2001 Message-Id: <b8ed8ed29d239693e2b32b62cbac3b2847937b9b.1394648513.git.daniel@totalueberwachung.de> From: Alexander Chemeris <Alexander.Chemeris@gmail.com> Date: Fri, 7 Mar 2014 21:03:44 +0100 Subject: [PATCH 1/1] 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, 23 insertions(+), 8 deletions(-)
diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index bb59a10..e199b6a 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++); - 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.
+ 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,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.
/* 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
+ * 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. Regards, Daniel -- - Daniel Willmann <dwillmann@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Schivelbeiner Str. 5 * 10439 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
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
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
-- Regards, Alexander Chemeris. CEO, Fairwaves, Inc. / ООО УмРадио https://fairwaves.co
Dear Alexander, while searching for a portable way to get the gmt offset I have found out that your patches set the wrong semi-octet to de/encode the sign. 3GPP TS 03.40 ch. 9.2.3.11 specifies that "bit 3 of the seventh octet of the TP-Service-Centre-Time-Stamp field [...] represents the algebraic sign of this difference." This is because the more significant digit is encoded in bits 0-3. Currently a two hour time offset (8*15min) will beencoded as 0x80 which will be decoded as -0. Please take a look at my two patches and squash them into yours. [FIX 1/2] fixup! sms: Fix gsm340_scts() to correctly decode absolute [FIX 2/2] fixup! sms: Fix support of negative timezone offsets in Regards, Daniel
--- src/gsm/gsm0411_utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index 197f2c3..3052dd7 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -120,8 +120,8 @@ time_t gsm340_scts(uint8_t *scts) /* according to gsm 03.40 time zone is "expressed in quarters of an hour" */ tz = *scts++; - ofs = gsm411_unbcdify(tz&0x7f) * 15*60; - if (tz&0x80) + ofs = gsm411_unbcdify(tz&0xf7) * 15*60; + if (tz&0x08) 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 -- 1.8.4.2
--- src/gsm/gsm0411_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index 3052dd7..b8a7b10 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -89,7 +89,7 @@ void gsm340_gen_scts(uint8_t *scts, time_t time) if (tm->tm_gmtoff >= 0) *scts++ = gsm411_bcdify(tm->tm_gmtoff/(60*15)); else - *scts++ = gsm411_bcdify(-tm->tm_gmtoff/(60*15)) | 0x80; + *scts++ = gsm411_bcdify(-tm->tm_gmtoff/(60*15)) | 0x08; #else #warning find a portable way to obtain timezone offset *scts++ = 0; -- 1.8.4.2
On Wed, 2014-03-26 at 22:44, Daniel Willmann wrote:
Please take a look at my two patches and squash them into yours.
Almost forgot: You'll need to update the test as well. Daniel -- - Daniel Willmann <dwillmann@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Schivelbeiner Str. 5 * 10439 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
participants (2)
-
Alexander Chemeris -
Daniel Willmann