From: Sylvain Munaut tnt@246tNt.com
The previous code only sent the FILE_ID tag data part, but according to the GSM 12.21 spec, section 8.3.6, the full SW Description 'object' must be sent so that includes the NM_ATT_SW_DESCR tag, the whole FILE_ID and the whole FILE_VERSION (including tags & length fields).
Note that functionnaly on a nanoBTS 139 I couldn't see any difference ... whatever I send in there it works ...
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/abis_nm.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/openbsc/src/abis_nm.c b/openbsc/src/abis_nm.c index 35ed8db..a450452 100755 --- a/openbsc/src/abis_nm.c +++ b/openbsc/src/abis_nm.c @@ -857,6 +857,7 @@ static int abis_nm_rx_sw_act_req(struct msgb *mb) const u_int8_t *sw_config; int sw_config_len; int file_id_len; + int file_ver_len; int nack = 0; int ret;
@@ -889,18 +890,24 @@ static int abis_nm_rx_sw_act_req(struct msgb *mb)
if (sw_config[0] != NM_ATT_SW_DESCR) DEBUGP(DNM, "SW_DESCR attribute identifier not found!\n"); + if (sw_config[1] != NM_ATT_FILE_ID) DEBUGP(DNM, "FILE_ID attribute identifier not found!\n"); file_id_len = sw_config[2] * 256 + sw_config[3];
+ if (sw_config[4+file_id_len] != NM_ATT_FILE_VERSION) + DEBUGP(DNM, "FILE_VERSION attribute identifier not found!\n"); + file_ver_len = sw_config[5+file_id_len] * 256 + sw_config[6+file_id_len]; + /* Assumes first SW file in list is the one to be activated */ - /* sw_config + 4 to skip over 2 attribute ID bytes and 16-bit length field */ + /* The +7 in length is to account for the 3 tags and 2 length fields */ + /* (see GSM 12.21 - 9.4.{62,18,19} for details */ return ipacc_sw_activate(mb->trx->bts, foh->obj_class, foh->obj_inst.bts_nr, foh->obj_inst.trx_nr, foh->obj_inst.ts_nr, - sw_config + 4, - file_id_len); + sw_config, + 7 + file_id_len + file_ver_len); }
/* Receive a CHANGE_ADM_STATE_ACK, parse the TLV and update local state */
Hi Sylvain,
instead of building more hacks on existing hacks, we should probably consider to use our TLV parser instead of things like:
if (sw_config[0] != NM_ATT_SW_DESCR) DEBUGP(DNM, "SW_DESCR attribute identifier not found!\n");
if (sw_config[1] != NM_ATT_FILE_ID) DEBUGP(DNM, "FILE_ID attribute identifier not found!\n"); file_id_len = sw_config[2] * 256 + sw_config[3];
if (sw_config[4+file_id_len] != NM_ATT_FILE_VERSION)
DEBUGP(DNM, "FILE_VERSION attribute identifier not found!\n");file_ver_len = sw_config[5+file_id_len] * 256 + sw_config[6+file_id_len];
all that we need is a second 'struct tlv_parsed' on the stack, as well as a 'static const struct tlv_definition' for the nested attributes such as SW_DESCR and FILE_ID, FILE_VERSION (or any others, if they exist).
If you have the time, I would appreciate a patch that solves the problem this way, rather than manually calculating lengths and indexing into sw_config[], etc.
The existing software activation code probably predates our tlv_parser and thus wasn't written properly in the first place.
Hi Harald,
instead of building more hacks on existing hacks, we should probably consider to use our TLV parser instead
[... patch snipped ...]
all that we need is a second 'struct tlv_parsed' on the stack, as well as a 'static const struct tlv_definition' for the nested attributes such as SW_DESCR and FILE_ID, FILE_VERSION (or any others, if they exist).
Mmmm, I don't really see how to do that without another hack ...
* You can view it either as flat (liek the wireshard dissector does) like this :
SW_CONFIG FILE_ID len_16 data FILE_VERSION len_16 data SW_CONFIG FILE_ID len_16 data FILE_VERSION len_16 data
But then the problem is that the tag will override each other in the tlv_parser, you can't really 'choose' which config you get.
* Or you can view the FILE_ID & FILE_VERSION as 'nested into' SW_CONFIG:
SW_CONFIG FILE_ID len_16 data FILE_VERSION len_16 data SW_CONFIG FILE_ID len_16 data FILE_VERSION len_16 data
But that's not better, because even if I start tlv_parsing a sw_config+1, SW_CONFIG tag doesn't have a 'length' it's just defined as having two child, so I can't really tell the tlv_parser to stop and it will continue past the first FILE_VERSION and into the second SW_CONFIG tag.
Or am I missing something obvious here ?
Sylvain
-
246tnt＠gmail.com -
Harald Welte -
Sylvain Munaut