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

[Alexander Chemeris]

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 at gmail.com> wrote:
> On Wed, Mar 12, 2014 at 10:53 PM, Daniel Willmann <dwillmann at 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