From patchwork Thu Mar 13 08:37:07 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Chemeris X-Patchwork-Id: 329805 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ganesha.gnumonks.org (ganesha.gnumonks.org [IPv6:2001:780:45:1d:225:90ff:fe52:c662]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 59EF22C008F for ; Thu, 13 Mar 2014 19:37:58 +1100 (EST) Received: from localhost ([127.0.0.1] helo=ganesha.gnumonks.org) by ganesha.gnumonks.org with esmtp (Exim 4.72) (envelope-from ) id 1WO198-0006uA-CD; Thu, 13 Mar 2014 09:37:46 +0100 Received: from mail-pb0-x234.google.com ([2607:f8b0:400e:c01::234]) by ganesha.gnumonks.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.72) (envelope-from ) id 1WO18s-0006u4-5R for openbsc@lists.osmocom.org; Thu, 13 Mar 2014 09:37:33 +0100 Received: by mail-pb0-f52.google.com with SMTP id rr13so783478pbb.39 for ; Thu, 13 Mar 2014 01:37:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=TycFvdVi5ufVC2Ajc1yF7xzPK1xyscOaW8xPBNv6Jcc=; b=pcKdx/o7GFo6+/L2a5EakuBaERc6wa+uUYCNswUW4vPNF8ZAkMtDVg+BRzHlsKuwmA AnhjFR8xegug6SNqKxzEu3zKbI9iv2uimZl/o4TKuURDLKaHHnn7bJqboFf3II/jJhqB UOAYCv00KkOz06BJDvFheAsEWAI2AJed9bsf3ed4akaqYI8xsuCnOS9ljg5JcAi6ek0Y EZgo9LnS1SqMPdrrRwXA62u7qarEESKnw7HjWJ+QCXl3mPnVKTa3lK6y8SWqxjSwZhyI tjgfngt4HxCjNTC8Pzh4Pr3hQOINaqksnIwHqL480pUlE4UOrkNFFWh1TJSNF7htch1e b04w== X-Received: by 10.68.58.34 with SMTP id n2mr781798pbq.122.1394699847772; Thu, 13 Mar 2014 01:37:27 -0700 (PDT) MIME-Version: 1.0 Received: by 10.70.88.239 with HTTP; Thu, 13 Mar 2014 01:37:07 -0700 (PDT) In-Reply-To: <20140312185313.GD2371@adrastea.totalueberwachung.de> References: <20140312185313.GD2371@adrastea.totalueberwachung.de> From: Alexander Chemeris Date: Thu, 13 Mar 2014 12:37:07 +0400 Message-ID: Subject: Re: [PATCH 6/6] sms: Fix gsm340_scts() to correctly decode absolute valid times. To: Daniel Willmann X-Spam-Score: -0.1 (/) Cc: OpenBSC Mailing List X-BeenThere: openbsc@lists.osmocom.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Development of the OpenBSC GSM base station controller List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: openbsc-bounces@lists.osmocom.org Errors-To: openbsc-bounces@lists.osmocom.org On Wed, Mar 12, 2014 at 10:53 PM, Daniel Willmann 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. From d3232c4c066497fa25365dd677069e24192e811a Mon Sep 17 00:00:00 2001 From: Alexander Chemeris 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