diff mbox

sms: Proper decoding and storage of SMS validity period.

Message ID CABmJbFVNzNLAyzTP0Ve4HBYkUk-mBJNW8wTvgy0yFHMU6czvCQ@mail.gmail.com
State New
Headers show

Commit Message

Alexander Chemeris March 12, 2014, 5:06 p.m. UTC
On Wed, Mar 12, 2014 at 8:02 PM, Daniel Willmann <dwillmann@sysmocom.de> wrote:
> On Wed, 2014-03-12 at 19:53, Alexander Chemeris wrote:
>> On Wed, Mar 12, 2014 at 6:59 PM, Daniel Willmann <dwillmann@sysmocom.de> wrote:
>> > looks all fine except for the function name. Something more expressive
>> > would be nice - how about gsm340_validity_period_as_ts()?
>>
>> I'm not sure "as_ts" is really more expressive,
>
>> gsm340_validity_period_unix_time() would be more expressive, but it's
>> also painfully long. I would suggest:
>>  - gsm340_validity_time()
>
> I like that one.

Updated

Comments

Alexander Chemeris March 16, 2014, 9:40 a.m. UTC | #1
Hi Holger,

it seems there are no more comments on this code. Could you please merge it?
It is independent of other changes.

On Wed, Mar 12, 2014 at 9:06 PM, Alexander Chemeris
<alexander.chemeris@gmail.com> wrote:
> On Wed, Mar 12, 2014 at 8:02 PM, Daniel Willmann <dwillmann@sysmocom.de> wrote:
>> On Wed, 2014-03-12 at 19:53, Alexander Chemeris wrote:
>>> On Wed, Mar 12, 2014 at 6:59 PM, Daniel Willmann <dwillmann@sysmocom.de> wrote:
>>> > looks all fine except for the function name. Something more expressive
>>> > would be nice - how about gsm340_validity_period_as_ts()?
>>>
>>> I'm not sure "as_ts" is really more expressive,
>>
>>> gsm340_validity_period_unix_time() would be more expressive, but it's
>>> also painfully long. I would suggest:
>>>  - gsm340_validity_time()
>>
>> I like that one.
>
> Updated
>
> --
> Regards,
> Alexander Chemeris.
> CEO, Fairwaves, Inc. / ООО УмРадио
> https://fairwaves.co
Holger Freyther March 16, 2014, 9:55 a.m. UTC | #2
On Sun, Mar 16, 2014 at 01:40:58PM +0400, Alexander Chemeris wrote:

Dear Alexander,

> it seems there are no more comments on this code. Could you please merge it?
> It is independent of other changes.

there is a fixed amount of time I can spend on review a week. You sadly
have used some of this time by sending not compiling patches, wrong ones,
etc. This means you will need to wait for my next timeslot.

The lack of comments doesn't mean that there will be none. E.g. in one
of the testcases I noticed that you don't use C99 initializers but the
old/error prone way of initializing. :)

holger

PS: I will not reply the same to all your other messages. ;)
Holger Freyther March 20, 2014, 9:20 p.m. UTC | #3
On Wed, Mar 12, 2014 at 09:06:29PM +0400, Alexander Chemeris wrote:

> +/* Decode validity period format 'relative in integer representation'.
> + * Returns number of seconds relative to a current time. */

this style of comments is only for net/* in the kernel coding style.


> +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;


What does this warning mean? The code will not be able to generate
and initiate the RP-Error procedure. So how do you intend to indicate
the error condition? What should the caler do?
Daniel Willmann March 26, 2014, 2:14 p.m. UTC | #4
Hi Holger,

On Thu, 2014-03-20 at 22:20, Holger Hans Peter Freyther wrote:
> On Wed, Mar 12, 2014 at 09:06:29PM +0400, Alexander Chemeris wrote:
> 
> > +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;
> 
> What does this warning mean? The code will not be able to generate
> and initiate the RP-Error procedure. So how do you intend to indicate
> the error condition? What should the caler do?

The GSM spec wants us to send back an RP-Error in this case as well as
in the default: of gsm340_validity_period(). The latter already has a
FIXME comment in it and I just wanted to make sure we don't forget this
case if we implement the rp-error.
That's why I asked that we either put a FIXME or #warning here.

I think we can signal an error condition by returning 0 (both in
gsm340_vp_relative_integer() and in gsm340_validity_time() and the
caller of gsm340_validity_time() can then deal with the error and return
a proper RP-Error in the future.

Thinking about it I guess we should implement it like that right now.


Regards,
Daniel
diff mbox

Patch

From e6475447bfbffde4cf03b930f791ae0e9540428d Mon Sep 17 00:00:00 2001
From: Alexander Chemeris <Alexander.Chemeris@gmail.com>
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 |   11 ++++-
 src/gsm/gsm0411_utils.c             |   91 +++++++++++++++++------------------
 src/gsm/libosmogsm.map              |    1 +
 3 files changed, 54 insertions(+), 49 deletions(-)

diff --git a/include/osmocom/gsm/gsm0411_utils.h b/include/osmocom/gsm/gsm0411_utils.h
index ad368e6..f563158 100644
--- a/include/osmocom/gsm/gsm0411_utils.h
+++ b/include/osmocom/gsm/gsm0411_utils.h
@@ -2,6 +2,10 @@ 
 #define _GSM0411_UTILS_H
 
 #include <time.h>
+#include <osmocom/core/defs.h>
+
+/* 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 +21,11 @@  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_time(time_t now, uint8_t sms_vpf, uint8_t *sms_vp);
+/* decode validity period. return relative minutes */
+unsigned long gsm340_validity_period(uint8_t sms_vpf, uint8_t *sms_vp)
+	OSMO_DEPRECATED("Use gsm340_validity_time() instead.");
 
 /* 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..e0908ef 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 <zecke@selfish.org>
  * (C) 2010 by On-Waves
  * (C) 2011 by Andreas Eversberg <jolly@eversberg.eu>
+ * (C) 2014 by Alexander Chemeris <Alexander.Chemeris@fairwaves.co>
  *
  * All Rights Reserved
  *
@@ -34,6 +35,7 @@ 
 
 #include <osmocom/gsm/gsm48.h>
 #include <osmocom/gsm/gsm_utils.h>
+#include <osmocom/gsm/gsm0411_utils.h>
 #include <osmocom/gsm/protocol/gsm_03_40.h>
 #include <osmocom/gsm/protocol/gsm_04_11.h>
 
@@ -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_time(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,26 +196,34 @@  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;
 	}
 }
 
+/* Decode validity period. return relative minutes.
+ * This behaviour is broken, but we're mimicing to it for compatibility. */
+unsigned long gsm340_validity_period(uint8_t sms_vpf, uint8_t *sms_vp)
+{
+	time_t now = time(NULL);
+	return (gsm340_validity_time(now, sms_vpf, sms_vp) - now) / 60;
+}
+
 /* 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/libosmogsm.map b/src/gsm/libosmogsm.map
index 9d15d66..c337def 100644
--- a/src/gsm/libosmogsm.map
+++ b/src/gsm/libosmogsm.map
@@ -66,6 +66,7 @@  gsm340_gen_oa;
 gsm340_gen_scts;
 gsm340_scts;
 gsm340_validity_period;
+gsm340_validity_time;
 
 gsm411_bcdify;
 gsm411_msgb_alloc;
-- 
1.7.9.5