7 Mar
2014
7 Mar
'14
9:25 p.m.
- Support negative timezone offsets decoding.
- Correctly account timezone offset and artificial offset mktime()
introduces.
--
Regards,
Alexander Chemeris.
CEO, Fairwaves, Inc. / ООО УмРадио
https://fairwaves.co
12 Mar
12 Mar
7:53 p.m.
New subject: [PATCH 6/6] sms: Fix gsm340_scts() to correctly decode absolute valid times.
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(a)totalueberwachung.de>
From: Alexander Chemeris <Alexander.Chemeris(a)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(a)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
13 Mar
13 Mar
9:37 a.m.
New subject: [PATCH 6/6] sms: Fix gsm340_scts() to correctly decode absolute valid times.
On Wed, Mar 12, 2014 at 10:53 PM, Daniel Willmann <dwillmann(a)sysmocom.de> wrote:
Fixed.
Thanks.
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
Hello Alexander,
On Fri, 2014-03-07 at 21:25, Alexander Chemeris wrote:
Yeah, a leftover from an intermediate implementation I had. Fixed.
@@ -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. @@ -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.
16 Mar
16 Mar
10:46 a.m.
New subject: [PATCH 6/6] sms: Fix gsm340_scts() to correctly decode absolute valid times.
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(a)gmail.com> wrote:
On Wed, Mar 12, 2014 at 10:53 PM, Daniel Willmann
<dwillmann(a)sysmocom.de> wrote:
Fixed.
Thanks.
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
Hello Alexander,
On Fri, 2014-03-07 at 21:25, Alexander Chemeris wrote:
Yeah, a leftover from an intermediate implementation I had. Fixed.
@@ -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. @@
-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.
26 Mar
26 Mar
10:44 p.m.
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
10:44 p.m.
New subject: [FIX 1/2] fixup! sms: Fix gsm340_scts() to correctly decode absolute valid times.
---
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
10:44 p.m.
New subject: [FIX 2/2] fixup! sms: Fix support of negative timezone offsets in gsm340_gen_scts().
---
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
10:56 p.m.
New subject: your mail
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(a)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
3695
days inactive
3714
days old
7 comments
2 participants
participants (2)
-
Alexander Chemeris -
Daniel Willmann