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

[Daniel Willmann]

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 at totalueberwachung.de>
> From: Alexander Chemeris <Alexander.Chemeris at 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 at 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