Hello!

> thanks a lot, I am afraid we need one more round.

No problem :)

> -#define tmsi_from_string(str) strtoul(str, NULL, 10)
> +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
>
> this macro is used for parsing strings from the network. We should not modify it.

I cannot find where...

Searching 420 files for "tmsi_from_string" (regex)

/home/wmn/openbsc/openbsc/include/openbsc/gsm_subscriber.h:
   15  #define GSM_SUBSCRIBER_FIRST_CONTACT    0x00000001
   16  /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */
   17: #define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
   18 
   19  #define GSM_SUBSCRIBER_NO_EXPIRATION    0x0

/home/wmn/openbsc/openbsc/src/libcommon/gsm_subscriber_base.c:
  122  {
  123      struct gsm_subscriber *subscr;
  124:     uint8_t tmsi_val = tmsi_from_string(tmsi);
  125 
  126      llist_for_each_entry(subscr, subscr_bsc_active_subscribers(), entry) {

/home/wmn/openbsc/openbsc/src/libmsc/db.c:
  890      string = dbi_result_get_string(result, "tmsi");
  891      if (string)
  892:         subscr->tmsi = tmsi_from_string(string);
  893 
  894      string = dbi_result_get_string(result, "name");

/home/wmn/openbsc/openbsc/src/libmsc/gsm_subscriber.c:
  215  {
  216      struct gsm_subscriber *subscr;
  217:     uint32_t tmsi_val = tmsi_from_string(tmsi);
  218 
  219      /* we might have a record in memory already */

/home/wmn/openbsc/openbsc/tests/gsm0408/gsm0408_test.c:
   94      mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi);
   95      gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2);
   96:     COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi);
   97 
   98      /* imsi code */

5 matches across 5 files

> struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group *sgrp,
> -                                          uint32_t tmsi)
> +                                          const char *tmsi)
>
> why? the number of bytes needed fit in uint32_t so we should remain with this internal storage. We should just make sure to always print it as hex.

Ok, I'll revert this changes.

> -#define SCHEMA_REVISION "4"
> +#define SCHEMA_REVISION "5"
>
> good, but I think we should change the schema to use uint32_t/INTEGER for the TMSI instead of text.

I'll do it in db.c.
 

С наилучшими пожеланиями,
Яницкий Вадим.

2016-03-27 22:44 GMT+06:00 Holger Freyther <holger@freyther.de>:

> On 27 Mar 2016, at 14:17, Vadim Yanitskiy <axilirator@gmail.com> wrote:

Dear Vadim,


>
> In OpenBSC, we traditionally displayed a TMSI in its integer
> representation, which is quite unusual in the telecom world. A TMSI is
> normally printed as a series of 8 hex digits.
>
> This patch aligns OpenBSC with the telecom industry standard.

thanks a lot, I am afraid we need one more round.


> -#define tmsi_from_string(str) strtoul(str, NULL, 10)
> +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)

this macro is used for parsing strings from the network. We should not modify it.



> struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group *sgrp,
> -                                          uint32_t tmsi)
> +                                          const char *tmsi)


why? the number of bytes needed fit in uint32_t so we should remain with this internal storage. We should just make sure to always print it as hex.


> -#define SCHEMA_REVISION "4"
> +#define SCHEMA_REVISION "5"

good, but I think we should change the schema to use uint32_t/INTEGER for the TMSI instead of text.



> +             /* Update old TMSI */
> +             update = dbi_conn_queryf(conn,
> +                             "UPDATE Subscriber "
> +                             "SET tmsi = '0x%08x' "
> +                             "WHERE id = %llu",

but to int :)


kind regards
        holger