From patchwork Tue Mar 11 20:44:24 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Chemeris X-Patchwork-Id: 329215 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 8404E2C00CE for ; Wed, 12 Mar 2014 07:52:21 +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 1WNTeh-0003ny-Sj; Tue, 11 Mar 2014 21:52:08 +0100 Received: from mail-pd0-x22d.google.com ([2607:f8b0:400e:c02::22d]) by ganesha.gnumonks.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.72) (envelope-from ) id 1WNTeM-0003np-I3 for openbsc@lists.osmocom.org; Tue, 11 Mar 2014 21:51:50 +0100 Received: by mail-pd0-f173.google.com with SMTP id z10so81233pdj.4 for ; Tue, 11 Mar 2014 13:51:44 -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=I7llDKyc4zNn6GHIpgZYRBT8bJKwnJ7Ri4Nmk1ImOLQ=; b=Zc1uM1X8MvMGrtpss4VFjxZhaxVLgyUGLMPXrQeimot1NFTqvPZrqYwv7cW/dkS8Kv otLGJsvPpyqxNhFT6opD/kO4SameOw2kXHxzK4DZz+gH/74EqnAFsPxzeelfc/b7nTSs pIxmdF9iZxvf1nVQDlhd+IW7JRjk25+7AesmbKZODloUZjoicogmtg3hSlkqarsVKuXh bEOqsI+xk9VzYGm4LcudEBiBndbc9PKOqFo3RgnnhYopjDbitpZR3pJwvA7V1MUByaJo zUGItE2Pl0yYg0nCWY/8SZQs3HQ3l3UUp8Bu0SPlErfsLLeEbfIEPkvp83xyRxmRLqXb zUoA== X-Received: by 10.68.196.137 with SMTP id im9mr258125pbc.105.1394571104010; Tue, 11 Mar 2014 13:51:44 -0700 (PDT) MIME-Version: 1.0 Received: by 10.70.88.239 with HTTP; Tue, 11 Mar 2014 13:44:24 -0700 (PDT) In-Reply-To: <20140311192602.GE27543@adrastea.totalueberwachung.de> References: <20140311192602.GE27543@adrastea.totalueberwachung.de> From: Alexander Chemeris Date: Wed, 12 Mar 2014 00:44:24 +0400 Message-ID: Subject: Re: [PATCH] sms: Proper decoding and storage of SMS validity period. 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 Hi Daniel, On Tue, Mar 11, 2014 at 11:26 PM, Daniel Willmann wrote: > On Fri, 2014-03-07 at 20:11, Alexander Chemeris wrote: >> -unsigned long gsm340_validity_period(uint8_t sms_vpf, uint8_t *sms_vp) >> +time_t gsm340_validity_period(time_t now, uint8_t sms_vpf, uint8_t *sms_vp) >> { >> uint8_t fi; /* functionality indicator */ >> >> switch (sms_vpf) { >> case GSM340_TP_VPF_RELATIVE: >> - return gsm340_vp_relative(sms_vp); >> + return gsm340_vp_relative(now, sms_vp); > > You could also omit now in all the static function and return now + > func() in gsm340_validity_period(). > That looks slightly cleaner to me and shouldn't change testability. I made it this way to follow a principle of least surprise - you know, that you always get absolute time as a return value and you don't even need to think about this. But that said, this is a minor issue to me, so I've changed it. All other comments are proper and are also fixed in the attached patch and in the branch (achemeris/sms-validity). From 3bb39fbda84fe1081ab4d936520ea906fd069d06 Mon Sep 17 00:00:00 2001 From: Alexander Chemeris Date: Tue, 26 Nov 2013 16:43:10 -0600 Subject: [PATCH 1/6] sms: Proper decoding and storage of SMS validity period. - Use time_t instead of abstract minutes to store validity time to make sure we support all possible values. - Return UNIX time from it to support both relative and absolute expire times. - Pass a current time to gsm340_validity_period() to compute the absolute expire time if a relative time is specified. What time is "current" is to be decided by callers. - We also introduce SMS_DEFAULT_VALIDITY_PERIOD macros to reduce number of magic values. --- include/osmocom/gsm/gsm0411_utils.h | 7 ++- src/gsm/gsm0411_utils.c | 83 +++++++++++++++-------------------- 2 files changed, 41 insertions(+), 49 deletions(-) diff --git a/include/osmocom/gsm/gsm0411_utils.h b/include/osmocom/gsm/gsm0411_utils.h index ad368e6..cad3625 100644 --- a/include/osmocom/gsm/gsm0411_utils.h +++ b/include/osmocom/gsm/gsm0411_utils.h @@ -3,6 +3,9 @@ #include +/* Default SMS validity period is 2 days */ +#define SMS_DEFAULT_VALIDITY_PERIOD (2 * 24 * 60 * 60) + /* Turn int into semi-octet representation: 98 => 0x89 */ uint8_t gsm411_bcdify(uint8_t value); @@ -17,8 +20,8 @@ void gsm340_gen_scts(uint8_t *scts, time_t time); /* Decode 03.40 TP-SCTS (into utc/gmt timestamp) */ time_t gsm340_scts(uint8_t *scts); -/* decode validity period. return minutes */ -unsigned long gsm340_validity_period(uint8_t sms_vpf, uint8_t *sms_vp); +/* decode validity period. return absolute time */ +time_t gsm340_validity_period(time_t now, uint8_t sms_vpf, uint8_t *sms_vp); /* determine coding alphabet dependent on GSM 03.38 Section 4 DCS */ enum sms_alphabet gsm338_get_sms_alphabet(uint8_t dcs); diff --git a/src/gsm/gsm0411_utils.c b/src/gsm/gsm0411_utils.c index a8ba810..9189b3e 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -7,6 +7,7 @@ * (C) 2010-2013 by Holger Hans Peter Freyther * (C) 2010 by On-Waves * (C) 2011 by Andreas Eversberg + * (C) 2014 by Alexander Chemeris * * All Rights Reserved * @@ -34,6 +35,7 @@ #include #include +#include #include #include @@ -120,21 +122,13 @@ time_t gsm340_scts(uint8_t *scts) return mktime(&tm) - ofs; } -/* Return the default validity period in minutes */ -static unsigned long gsm340_vp_default(void) -{ - unsigned long minutes; - /* Default validity: two days */ - minutes = 24 * 60 * 2; - return minutes; -} - -/* Decode validity period format 'relative' */ -static unsigned long gsm340_vp_relative(uint8_t *sms_vp) +/* Decode validity period format 'relative'. + * Returns number of seconds relative to a current time. */ +static time_t gsm340_vp_relative(uint8_t *sms_vp) { /* Chapter 9.2.3.12.1 */ uint8_t vp; - unsigned long minutes; + time_t minutes; vp = *(sms_vp); if (vp <= 143) @@ -145,58 +139,53 @@ static unsigned long gsm340_vp_relative(uint8_t *sms_vp) minutes = vp-166 * 60 * 24; else minutes = vp-192 * 60 * 24 * 7; - return minutes; + + /* Convert to seconds */ + return minutes * 60; } -/* Decode validity period format 'absolute' */ -static unsigned long gsm340_vp_absolute(uint8_t *sms_vp) +/* Decode validity period format 'absolute'. + * Returns UNIX time. */ +static time_t gsm340_vp_absolute(uint8_t *sms_vp) { /* Chapter 9.2.3.12.2 */ - time_t expires, now; - unsigned long minutes; - - expires = gsm340_scts(sms_vp); - now = time(NULL); - if (expires <= now) - minutes = 0; - else - minutes = (expires-now)/60; - return minutes; + return gsm340_scts(sms_vp); } -/* Decode validity period format 'relative in integer representation' */ -static unsigned long gsm340_vp_relative_integer(uint8_t *sms_vp) +/* Decode validity period format 'relative in integer representation'. + * Returns number of seconds relative to a current time. */ +static time_t gsm340_vp_relative_integer(uint8_t *sms_vp) { uint8_t vp; - unsigned long minutes; vp = *(sms_vp); if (vp == 0) { LOGP(DLSMS, LOGL_ERROR, "reserved relative_integer validity period\n"); - return gsm340_vp_default(); +#warning We should return an RP-Error here. + return SMS_DEFAULT_VALIDITY_PERIOD; } - minutes = vp/60; - return minutes; + return vp; } -/* Decode validity period format 'relative in semi-octet representation' */ -static unsigned long gsm340_vp_relative_semioctet(uint8_t *sms_vp) +/* Decode validity period format 'relative in semi-octet representation'. + * Returns number of seconds relative to a current time. */ +static time_t gsm340_vp_relative_semioctet(uint8_t *sms_vp) { - unsigned long minutes; - minutes = gsm411_unbcdify(*sms_vp++)*60; /* hours */ - minutes += gsm411_unbcdify(*sms_vp++); /* minutes */ - minutes += gsm411_unbcdify(*sms_vp++)/60; /* seconds */ - return minutes; + time_t hours, minutes, seconds; + hours = gsm411_unbcdify(*sms_vp++); /* hours */ + minutes = gsm411_unbcdify(*sms_vp++); /* minutes */ + seconds = gsm411_unbcdify(*sms_vp++); /* seconds */ + return hours*60*60 + minutes*60 + seconds; } -/* decode validity period. return minutes */ -unsigned long gsm340_validity_period(uint8_t sms_vpf, uint8_t *sms_vp) +/* Decode validity period. Returns absolute UNIX time. */ +time_t gsm340_validity_period(time_t now, uint8_t sms_vpf, uint8_t *sms_vp) { uint8_t fi; /* functionality indicator */ switch (sms_vpf) { case GSM340_TP_VPF_RELATIVE: - return gsm340_vp_relative(sms_vp); + return now + gsm340_vp_relative(sms_vp); case GSM340_TP_VPF_ABSOLUTE: return gsm340_vp_absolute(sms_vp); case GSM340_TP_VPF_ENHANCED: @@ -207,23 +196,23 @@ unsigned long gsm340_validity_period(uint8_t sms_vpf, uint8_t *sms_vp) /* read validity period format */ switch (fi & 0x7) { case 0x0: - return gsm340_vp_default(); /* no vpf specified */ + return now + SMS_DEFAULT_VALIDITY_PERIOD; /* no vpf specified */ case 0x1: - return gsm340_vp_relative(sms_vp); + return now + gsm340_vp_relative(sms_vp); case 0x2: - return gsm340_vp_relative_integer(sms_vp); + return now + gsm340_vp_relative_integer(sms_vp); case 0x3: - return gsm340_vp_relative_semioctet(sms_vp); + return now + gsm340_vp_relative_semioctet(sms_vp); default: /* The GSM spec says that the SC should reject any unsupported and/or undefined values. FIXME */ LOGP(DLSMS, LOGL_ERROR, "Reserved enhanced validity period format\n"); - return gsm340_vp_default(); + return now + SMS_DEFAULT_VALIDITY_PERIOD; } case GSM340_TP_VPF_NONE: default: - return gsm340_vp_default(); + return now + SMS_DEFAULT_VALIDITY_PERIOD; } } -- 1.7.9.5