Patchwork [6/6] sms: Fix gsm340_scts() to correctly decode absolute valid times.

login
register
mail settings
Submitter Alexander Chemeris
Date March 13, 2014, 8:37 a.m.
Message ID <CABmJbFWC7ztmWHk87-j8Wn8VbPN3S--UdOXAfTNv7bsKVi1kyA@mail.gmail.com>
Download mbox | patch
Permalink /patch/329805/
State New
Headers show

Comments

Alexander Chemeris - March 13, 2014, 8:37 a.m.
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.
Alexander Chemeris - March 16, 2014, 9:46 a.m.
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

Patch

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