Hello,

a few comments inline.

On Fri, 2014-03-07 at 20:11, Alexander Chemeris wrote:

PS This is a first patch of a series of patches which fix SMS validity time decoding. I'm cleaning the patch set and want to see if this patch is fine, as other patches rely on it.

From 840b7ca3efa834a0d02e4e32f62d3dac63c6bac6 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] 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 | 62 +++++++++++++++-------------------- 2 files changed, 31 insertions(+), 38 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 <time.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 +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..1a6862b 100644 --- a/src/gsm/gsm0411_utils.c +++ b/src/gsm/gsm0411_utils.c @@ -34,6 +34,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>

@@ -121,16 +122,14 @@ time_t gsm340_scts(uint8_t *scts) }

/* Return the default validity period in minutes */

You need to update the comment.

-static unsigned long gsm340_vp_default(void) +static time_t gsm340_vp_default(time_t now) {

• unsigned long minutes; /* Default validity: two days */
• minutes = 24 * 60 * 2;
• return minutes;

• return now + SMS_DEFAULT_VALIDITY_PERIOD;

}

/* Decode validity period format 'relative' */ -static unsigned long gsm340_vp_relative(uint8_t *sms_vp) +static unsigned long gsm340_vp_relative(time_t now, uint8_t *sms_vp)

You should probably return time_t here as well. Also the variable minutes should become time_t as well.

{ /* Chapter 9.2.3.12.1 */ uint8_t vp; @@ -145,58 +144,49 @@ static unsigned long gsm340_vp_relative(uint8_t *sms_vp) minutes = vp-166 * 60 * 24; else minutes = vp-192 * 60 * 24 * 7;

• return minutes;

• /* Calculate absolute time from the relative offset */
• return now + minutes * 60;

}

/* Decode validity period format 'absolute' */ -static unsigned long gsm340_vp_absolute(uint8_t *sms_vp) +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) +static time_t gsm340_vp_relative_integer(time_t now, 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();
  

• return gsm340_vp_default(now);
  

Please add a comment (with FIXME) or #warning that we should return an RP-Error here.

}

• minutes = vp/60;
• return minutes;

• return now + vp;

}

/* Decode validity period format 'relative in semi-octet representation' */ -static unsigned long gsm340_vp_relative_semioctet(uint8_t *sms_vp) +static time_t gsm340_vp_relative_semioctet(time_t now, 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;

• unsigned long hours, minutes, seconds;

time_t?

• hours = gsm411_unbcdify(*sms_vp++); /* hours */
• minutes = gsm411_unbcdify(*sms_vp++); /* minutes */
• seconds = gsm411_unbcdify(*sms_vp++); /* seconds */
• return now + hours*60*60 + minutes*60 + seconds;

}

/* decode validity period. return minutes */

The comment is wrong now

-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.

case GSM340_TP_VPF_ABSOLUTE: return gsm340_vp_absolute(sms_vp); case GSM340_TP_VPF_ENHANCED: @@ -207,23 +197,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 gsm340_vp_default(now); /* no vpf specified */
  

  case 0x1:

• 	return gsm340_vp_relative(sms_vp);
  

• 	return gsm340_vp_relative(now, sms_vp);
  

  case 0x2:

• 	return gsm340_vp_relative_integer(sms_vp);
  

• 	return gsm340_vp_relative_integer(now, sms_vp);
  

  case 0x3:

• 	return gsm340_vp_relative_semioctet(sms_vp);
  

• 	return gsm340_vp_relative_semioctet(now, 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 gsm340_vp_default(now);
  

  } case GSM340_TP_VPF_NONE: default:

• return gsm340_vp_default();
  

• return gsm340_vp_default(now);
  

  }

}

-- 1.7.9.5

Regards, Daniel