This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Daniel Willmann dwillmann at sysmocom.deHello,
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 at 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
--
- Daniel Willmann <dwillmann at 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