Implement gsm340_{gen_,}scts() without using tm_gmtoff

List overview All Threads
Download

newer

older

3 OpenBSC moderator request(s)...

OpenBSC unsubscribe notification

Daniel Willmann

26 Mar 2014 26 Mar '14

11:30 p.m.

Hello, this patch series depends on two patches from Alexander (once they sign handling is fixed): sms: Fix gsm340_scts() to correctly decode absolute valid times. sms: Fix support of negative timezone offsets in gsm340_gen_scts(). The first patch adds a test case to check that gsm340_scts is the inverse of gsm340_gen_scts. The next patches add a helper function to calculate the gmt offset and use those in the *_scts functions. The helper function is somewhat ugly, but it is the only portable way I could find to get the GMT offset. Ideas 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

Show replies by thread

Daniel Willmann

26 Mar 26 Mar

11:30 p.m.

New subject: [PATCH 1/4] sms_test: Test that gsm340_scts(gsm340_gen_scts(time) == time

gsm340_scts() needs to be the inverse of gsm340_gen_scts() Test different timezones and iterate through a whole year (2013) in 30 min steps. Record the current test result --- tests/sms/sms_test.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/sms/sms_test.ok | 20 +++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/tests/sms/sms_test.c b/tests/sms/sms_test.c index cdd4158..8b19760 100644 --- a/tests/sms/sms_test.c +++ b/tests/sms/sms_test.c @@ -268,6 +268,64 @@ static void test_gen_oa(void) printf("Result: len(%d) data(%s)\n", len, osmo_hexdump(oa, len)); } +const char *tz_list[] = { + "UTC", + "Europe/London", + "Europe/Berlin", + "Europe/Athens", + "Europe/Moscow", + "Canada/Central", + "America/New_York", + "America/Los_Angeles", + NULL +}; + +static int test_scts_id_one_tz(void) +{ + uint8_t scts[7]; + time_t ts, ts_res, start_ts, end_ts; + + /* 2013-01-01 00:00:00 - 2014-01-01 01:00:00 increment in 30min steps */ + start_ts = 1356998400; + end_ts = 1388538000; + for (ts = start_ts; ts <= end_ts; ts += 1800) { + memset(scts, 0, sizeof(scts)); + gsm340_gen_scts(scts, ts); + ts_res = gsm340_scts(scts); + + if (ts_res != ts) { + printf("%li -> %s -> %li\n", ts, + osmo_hexdump_nospc(scts, sizeof(scts)), ts_res); + return ts; + } + } + return 0; +} + +static void test_scts_id(void) +{ + int i; + time_t ts; + const char *tz; + char *old_tz = getenv("TZ"); + + for (i = 0; ;i++) { + tz = tz_list[i]; + if (!tz) + break; + + printf("Testing gsm340_scts(gsm340_gen_scts(ts)) == ts " + "for TZ %s\n", tz); + setenv("TZ", tz, 1); + tzset(); + + ts = test_scts_id_one_tz(); + if (ts) + printf("Timezone %s failed at ts %li\n", tz, ts); + } + setenv("TZ", old_tz, 1); +} + int main(int argc, char** argv) { printf("SMS testing\n"); @@ -414,6 +472,7 @@ int main(int argc, char** argv) test_octet_return(); test_gen_oa(); + test_scts_id(); printf("OK\n"); return 0; diff --git a/tests/sms/sms_test.ok b/tests/sms/sms_test.ok index fa536ea..a3d816c 100644 --- a/tests/sms/sms_test.ok +++ b/tests/sms/sms_test.ok @@ -26,4 +26,24 @@ Result: len(12) data(14 a1 21 43 65 87 09 21 43 65 87 19 ) Result: len(2) data(00 91 ) Result: len(9) data(0e d0 4f 78 d9 2d 9c 0e 01 ) Result: len(12) data(14 d0 4f 78 d9 2d 9c 0e c3 e2 31 19 ) +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ UTC +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/London +1364691600 -> 31301310000000 -> 1364695200 +Timezone Europe/London failed at ts 1364691600 +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/Berlin +1364695200 -> 31301320000000 -> 1364698800 +Timezone Europe/Berlin failed at ts 1364695200 +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/Athens +1364698800 -> 31301330000000 -> 1364702400 +Timezone Europe/Athens failed at ts 1364698800 +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/Moscow +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Canada/Central +1362880800 -> 31300120000000 -> 1362884400 +Timezone Canada/Central failed at ts 1362880800 +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ America/New_York +1362880800 -> 31300120000000 -> 1362884400 +Timezone America/New_York failed at ts 1362880800 +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ America/Los_Angeles +1362880800 -> 31300120000000 -> 1362884400 +Timezone America/Los_Angeles failed at ts 1362880800 OK -- 1.8.4.2

Daniel Willmann

11:30 p.m.

New subject: [PATCH 2/4] gsm0411_utils: Add helper function to get the gmt offset of a time_t

The function uses localtime as well as gmtime to calculate the timezone offset at a specific time. This is useful for the *_scts functions so we're not dependent on non-portable features (tm_gmtoff). --- src/gsm/gsm0411_utils.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index ad9753e..d857e41 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -72,6 +72,24 @@ uint8_t gsm411_unbcdify(uint8_t value) return ret; } +/* Figure out the timezone offset in a portable way. + * The idea is to convert the time_t into local and UTC struct tm + * representations and then calculate the difference of both. */ +static time_t gmtoffset_from_ts(time_t time) +{ + struct tm tm_local, tm_utc; + time_t ts_local, ts_utc; + + localtime_r(&time, &tm_local); + gmtime_r(&time, &tm_utc); + tm_utc.tm_isdst = 0; + tm_local.tm_isdst = 0; + ts_utc = mktime(&tm_utc); + ts_local = mktime(&tm_local); + + return ts_local - ts_utc; +} + /* Generate 03.40 TP-SCTS */ void gsm340_gen_scts(uint8_t *scts, time_t time) { -- 1.8.4.2

Daniel Willmann

11:30 p.m.

New subject: [PATCH 3/4] gsm0411_utils: Fix timezone offs calculation in gsm340_gen_scts()

The current code uses gmtime which returns the time in UTC. This is not the intended behaviour (otherwise the offset could always be set to zero). Iinstead of relying on tm_gmtoff this patch uses the (portable) gmtoffset_from_ts() function to determine the current time offset. Update the test with the expected result --- src/gsm/gsm0411_utils.c | 33 +++++++++++++++++---------------- tests/sms/sms_test.ok | 12 ------------ 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index d857e41..c6d68bd 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -93,23 +93,24 @@ static time_t gmtoffset_from_ts(time_t time) /* Generate 03.40 TP-SCTS */ void gsm340_gen_scts(uint8_t *scts, time_t time) { - struct tm *tm = gmtime(&time); - - *scts++ = gsm411_bcdify(tm->tm_year % 100); - *scts++ = gsm411_bcdify(tm->tm_mon + 1); - *scts++ = gsm411_bcdify(tm->tm_mday); - *scts++ = gsm411_bcdify(tm->tm_hour); - *scts++ = gsm411_bcdify(tm->tm_min); - *scts++ = gsm411_bcdify(tm->tm_sec); -#ifdef HAVE_TM_GMTOFF_IN_TM - if (tm->tm_gmtoff >= 0) - *scts++ = gsm411_bcdify(tm->tm_gmtoff/(60*15)); + struct tm tm; + time_t gmtoffset; + + localtime_r(&time, &tm); + + *scts++ = gsm411_bcdify(tm.tm_year % 100); + *scts++ = gsm411_bcdify(tm.tm_mon + 1); + *scts++ = gsm411_bcdify(tm.tm_mday); + *scts++ = gsm411_bcdify(tm.tm_hour); + *scts++ = gsm411_bcdify(tm.tm_min); + *scts++ = gsm411_bcdify(tm.tm_sec); + + gmtoffset = gmtoffset_from_ts(time); + + if (gmtoffset >= 0) + *scts++ = gsm411_bcdify(gmtoffset/(60*15)); else - *scts++ = gsm411_bcdify(-tm->tm_gmtoff/(60*15)) | 0x08; -#else -#warning find a portable way to obtain timezone offset - *scts++ = 0; -#endif + *scts++ = gsm411_bcdify(-gmtoffset/(60*15)) | 0x08; } /* Decode 03.40 TP-SCTS (into utc/gmt timestamp) */ diff --git a/tests/sms/sms_test.ok b/tests/sms/sms_test.ok index a3d816c..a779f12 100644 --- a/tests/sms/sms_test.ok +++ b/tests/sms/sms_test.ok @@ -28,22 +28,10 @@ Result: len(9) data(0e d0 4f 78 d9 2d 9c 0e 01 ) Result: len(12) data(14 d0 4f 78 d9 2d 9c 0e c3 e2 31 19 ) Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ UTC Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/London -1364691600 -> 31301310000000 -> 1364695200 -Timezone Europe/London failed at ts 1364691600 Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/Berlin -1364695200 -> 31301320000000 -> 1364698800 -Timezone Europe/Berlin failed at ts 1364695200 Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/Athens -1364698800 -> 31301330000000 -> 1364702400 -Timezone Europe/Athens failed at ts 1364698800 Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/Moscow Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Canada/Central -1362880800 -> 31300120000000 -> 1362884400 -Timezone Canada/Central failed at ts 1362880800 Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ America/New_York -1362880800 -> 31300120000000 -> 1362884400 -Timezone America/New_York failed at ts 1362880800 Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ America/Los_Angeles -1362880800 -> 31300120000000 -> 1362884400 -Timezone America/Los_Angeles failed at ts 1362880800 OK -- 1.8.4.2

Daniel Willmann

11:30 p.m.

New subject: [PATCH 4/4] gsm0411_utils: Use portable helper gmtoffset_from_ts helper in gsm340_scts

Avoid depending on the presence of tm_gmtoff to calculate the gmt offset. --- src/gsm/gsm0411_utils.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index c6d68bd..94d48fd 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -119,7 +119,7 @@ time_t gsm340_scts(uint8_t *scts) struct tm tm; uint8_t yr, tz; int ofs; - time_t timestamp; + time_t timestamp, gmtoffset; memset(&tm, 0x00, sizeof(struct tm)); @@ -151,12 +151,12 @@ time_t gsm340_scts(uint8_t *scts) if (timestamp < 0) return -1; + gmtoffset = gmtoffset_from_ts(timestamp); + /* 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 + timestamp += gmtoffset; return timestamp; } -- 1.8.4.2

Alexander Chemeris

27 Mar 27 Mar

6:21 a.m.

New subject: [PATCH 1/4] sms_test: Test that gsm340_scts(gsm340_gen_scts(time) == time

Hi Daniel, On Thu, Mar 27, 2014 at 2:30 AM, Daniel Willmann <dwillmann(a)sysmocom.de> wrote:

...

+Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ UTC +Testing gsm340_scts(gsm340_gen_scts(ts)) == ts for TZ Europe/London +1364691600 -> 31301310000000 -> 1364695200 +Timezone Europe/London failed at ts 1364691600

A quick question - why does it says "failed" here? If it's ok, than we should change the wording of the message, I think. If it's not ok, we should not add this to "sms_test.ok". Right now it's confusing. -- Regards, Alexander Chemeris. CEO, Fairwaves, Inc. https://fairwaves.co

Daniel Willmann

9:18 p.m.

New subject: [PATCH 1/4] sms_test: Test that gsm340_scts(gsm340_gen_scts(time) == time

Hi Alexander, On Thu, 2014-03-27 at 09:21, Alexander Chemeris wrote:

...

On Thu, Mar 27, 2014 at 2:30 AM, Daniel Willmann <dwillmann(a)sysmocom.de> wrote:

you're right. I was trying to first record the status quo so you can see the behaviour changes as the issue is fixed. Holger was quite fond of this in the recent lapdm fixes, but it probably makes less sense here. Holger, any preferences how I should handle this? Move the test to the end, have a failing test, ..? Regard, 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

Holger Hans Peter Freyther

28 Mar 28 Mar

9:16 a.m.

New subject: [PATCH 1/4] sms_test: Test that gsm340_scts(gsm340_gen_scts(time) == time

On Thu, Mar 27, 2014 at 09:18:25PM +0100, Daniel Willmann wrote:

...

The question is if you want to have a known issue in new code or not. What is the impact for the end-user/NITB? How will this fail? Is this a step backward compared to the extensions by BSD/Linux? holger

3705

days inactive

3707

days old

openbsc@lists.osmocom.org

Manage subscription

7 comments

3 participants

tags (0)

participants (3)

Alexander Chemeris
Daniel Willmann
Holger Hans Peter Freyther