Change in osmo-bsc[master]: gsm_04_08: improve gsm48_multirate_config()
dexter has submitted this change and it was merged. ( https://gerrit.osmocom.org/11443 ) Change subject: gsm_04_08: improve gsm48_multirate_config() .. gsm_04_08: improve gsm48_multirate_config() The function gsm48_multirate_config() generates the multirate configuration IE, that is sent via RSL to configure the active set of AMR codecs inside the BTS. The function already works, but it does not check the input data for consistancy. Lets add some consistancy check to make sure that inconsistant parameters are rejected. Also allow the output pointer to be NULL, so that the function can be used to perform a dry run to be able to verify parameters. - Check for invalid / inconsistant configuration parameters - Perform a dry-run when lv pointer is set to NULL Change-Id: I06beb7dd7236c81c3a91af4d09c31891f4b910a4 Related: OS#3529 --- M include/osmocom/bsc/gsm_04_08_rr.h M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/gsm_04_08_rr.c M tests/gsm0408/Makefile.am M tests/gsm0408/gsm0408_test.c M tests/gsm0408/gsm0408_test.ok 6 files changed, 237 insertions(+), 26 deletions(-) Approvals: Jenkins Builder: Verified Harald Welte: Looks good to me, approved diff --git a/include/osmocom/bsc/gsm_04_08_rr.h b/include/osmocom/bsc/gsm_04_08_rr.h index e2e861d..8e4f787 100644 --- a/include/osmocom/bsc/gsm_04_08_rr.h +++ b/include/osmocom/bsc/gsm_04_08_rr.h @@ -23,7 +23,8 @@ struct msgb *msg, struct bsc_subscr *bsub); int gsm48_send_rr_classmark_enquiry(struct gsm_lchan *lchan); int gsm48_send_rr_ciph_mode(struct gsm_lchan *lchan, int want_imeisv); -int gsm48_multirate_config(uint8_t *lv, const struct amr_multirate_conf *mr, const struct amr_mode *modes); +int gsm48_multirate_config(uint8_t *lv, const struct gsm48_multi_rate_conf *mr_conf, + const struct amr_mode *modes, unsigned int num_modes); struct msgb *gsm48_make_ho_cmd(struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref); int gsm48_send_ho_cmd(struct gsm_lchan *old_lchan, struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref); diff --git a/src/osmo-bsc/bsc_vty.c b/src/osmo-bsc/bsc_vty.c index f156cc8..08b5dad 100644 --- a/src/osmo-bsc/bsc_vty.c +++ b/src/osmo-bsc/bsc_vty.c @@ -4391,11 +4391,12 @@ mr.ms_mode[0].mode = amr_mode; mr.bts_mode[0].mode = amr_mode; + mr.num_modes = 1; /* encode this configuration into the lchan for both uplink and * downlink direction */ - gsm48_multirate_config(lchan->mr_ms_lv, &mr, mr.ms_mode); - gsm48_multirate_config(lchan->mr_bts_lv, &mr, mr.bts_mode); + gsm48_multirate_config(lchan->mr_ms_lv, mr_conf, mr.ms_mode, mr.num_modes); + gsm48_multirate_config(lchan->mr_bts_lv, mr_conf, mr.bts_mode, mr.num_modes); return 0; } diff --git a/src/osmo-bsc/gsm_04_08_rr.c b/src/osmo-bsc/gsm_04_08_rr.c index 35044a3..4659c1a 100644 --- a/src/osmo-bsc/gsm_04_08_rr.c +++ b/src/osmo-bsc/gsm_04_08_rr.c @@ -321,47 +321,113 @@ cd->arfcn_lo = bts->c0->arfcn & 0xff; } -/*! \brief Encode a TS 04.08 multirate config LV according to 10.5.2.21aa - * \param[out] lv caller-allocated buffer of 7 bytes. First octet is IS length - * \param[in] mr multi-rate configuration to encode - * \param[in] modes array describing the AMR modes - * \returns 0 on success */ -int gsm48_multirate_config(uint8_t *lv, const struct amr_multirate_conf *mr, const struct amr_mode *modes) +/*! \brief Encode a TS 04.08 multirate config LV according to 10.5.2.21aa. + * \param[out] lv caller-allocated buffer of 7 bytes. First octet is is length. + * \param[in] mr_conf multi-rate configuration to encode (selected modes). + * \param[in] modes array describing the AMR modes. + * \param[in] num_modes length of the modes array. + * \returns 0 on success, -EINVAL on failure. */ +int gsm48_multirate_config(uint8_t *lv, + const struct gsm48_multi_rate_conf *mr_conf, + const struct amr_mode *modes, unsigned int num_modes) { - int num = 0, i; + int num = 0; + unsigned int i; + unsigned int k; + unsigned int m = 0; + bool mode_valid; + uint8_t *gsm48_ie = (uint8_t *) mr_conf; + const struct amr_mode *modes_selected[4]; + /* Check if modes for consistency (order and duplicates) */ + for (i = 0; i < num_modes; i++) { + if (i > 0 && modes[i - 1].mode > modes[i].mode) { + LOGP(DRR, LOGL_ERROR, +"BUG: Multirate codec with inconsistant config (mode order).\n"); + return -EINVAL; + } + if (i > 0 && modes[i - 1].mode == modes[i].mode) { + LOGP(DRR, LOGL_ERROR, +"BUG: Multirate codec with inconsistant config (duplicate modes).\n"); + return -
Change in osmo-bsc[master]: gsm_04_08: improve gsm48_multirate_config()
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11443 ) Change subject: gsm_04_08: improve gsm48_multirate_config() .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/11443 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06beb7dd7236c81c3a91af4d09c31891f4b910a4 Gerrit-Change-Number: 11443 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Comment-Date: Tue, 23 Oct 2018 18:21:30 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: gsm_04_08: improve gsm48_multirate_config()
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11443 to look at the new patch set (#2). Change subject: gsm_04_08: improve gsm48_multirate_config() .. gsm_04_08: improve gsm48_multirate_config() The function gsm48_multirate_config() generates the multirate configuration IE, that is sent via RSL to configure the active set of AMR codecs inside the BTS. The function already works, but it does not check the input data for consistancy. Lets add some consistancy check to make sure that inconsistant parameters are rejected. Also allow the output pointer to be NULL, so that the function can be used to perform a dry run to be able to verify parameters. - Check for invalid / inconsistant configuration parameters - Perform a dry-run when lv pointer is set to NULL Change-Id: I06beb7dd7236c81c3a91af4d09c31891f4b910a4 Related: OS#3529 --- M include/osmocom/bsc/gsm_04_08_rr.h M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/gsm_04_08_rr.c M tests/gsm0408/Makefile.am M tests/gsm0408/gsm0408_test.c M tests/gsm0408/gsm0408_test.ok 6 files changed, 237 insertions(+), 26 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/43/11443/2 -- To view, visit https://gerrit.osmocom.org/11443 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I06beb7dd7236c81c3a91af4d09c31891f4b910a4 Gerrit-Change-Number: 11443 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder (102)
Change in osmo-bsc[master]: gsm_04_08: improve gsm48_multirate_config()
dexter has uploaded this change for review. ( https://gerrit.osmocom.org/11443 Change subject: gsm_04_08: improve gsm48_multirate_config() .. gsm_04_08: improve gsm48_multirate_config() The function gsm48_multirate_config() generates the multirate configuration IE, that is sent via RSL to configure the active set of AMR codecs inside the BTS. The function already works, but it does not check the input data for consistancy. Lets add some consistancy check to make sure that inconsistant parameters are rejected. Also allow the output pointer to be NULL, so that the function can be used to perform a dry run to be able to verify parameters. - Check for invalid / inconsistant configuration parameters - Perform a dry-run when lv pointer is set to NULL Change-Id: I06beb7dd7236c81c3a91af4d09c31891f4b910a4 Related: OS#3529 --- M include/osmocom/bsc/gsm_04_08_rr.h M src/osmo-bsc/gsm_04_08_rr.c M tests/gsm0408/Makefile.am M tests/gsm0408/gsm0408_test.c M tests/gsm0408/gsm0408_test.ok 5 files changed, 234 insertions(+), 24 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/43/11443/1 diff --git a/include/osmocom/bsc/gsm_04_08_rr.h b/include/osmocom/bsc/gsm_04_08_rr.h index e2e861d..8e4f787 100644 --- a/include/osmocom/bsc/gsm_04_08_rr.h +++ b/include/osmocom/bsc/gsm_04_08_rr.h @@ -23,7 +23,8 @@ struct msgb *msg, struct bsc_subscr *bsub); int gsm48_send_rr_classmark_enquiry(struct gsm_lchan *lchan); int gsm48_send_rr_ciph_mode(struct gsm_lchan *lchan, int want_imeisv); -int gsm48_multirate_config(uint8_t *lv, const struct amr_multirate_conf *mr, const struct amr_mode *modes); +int gsm48_multirate_config(uint8_t *lv, const struct gsm48_multi_rate_conf *mr_conf, + const struct amr_mode *modes, unsigned int num_modes); struct msgb *gsm48_make_ho_cmd(struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref); int gsm48_send_ho_cmd(struct gsm_lchan *old_lchan, struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref); diff --git a/src/osmo-bsc/gsm_04_08_rr.c b/src/osmo-bsc/gsm_04_08_rr.c index 35044a3..4659c1a 100644 --- a/src/osmo-bsc/gsm_04_08_rr.c +++ b/src/osmo-bsc/gsm_04_08_rr.c @@ -321,47 +321,113 @@ cd->arfcn_lo = bts->c0->arfcn & 0xff; } -/*! \brief Encode a TS 04.08 multirate config LV according to 10.5.2.21aa - * \param[out] lv caller-allocated buffer of 7 bytes. First octet is IS length - * \param[in] mr multi-rate configuration to encode - * \param[in] modes array describing the AMR modes - * \returns 0 on success */ -int gsm48_multirate_config(uint8_t *lv, const struct amr_multirate_conf *mr, const struct amr_mode *modes) +/*! \brief Encode a TS 04.08 multirate config LV according to 10.5.2.21aa. + * \param[out] lv caller-allocated buffer of 7 bytes. First octet is is length. + * \param[in] mr_conf multi-rate configuration to encode (selected modes). + * \param[in] modes array describing the AMR modes. + * \param[in] num_modes length of the modes array. + * \returns 0 on success, -EINVAL on failure. */ +int gsm48_multirate_config(uint8_t *lv, + const struct gsm48_multi_rate_conf *mr_conf, + const struct amr_mode *modes, unsigned int num_modes) { - int num = 0, i; + int num = 0; + unsigned int i; + unsigned int k; + unsigned int m = 0; + bool mode_valid; + uint8_t *gsm48_ie = (uint8_t *) mr_conf; + const struct amr_mode *modes_selected[4]; + /* Check if modes for consistency (order and duplicates) */ + for (i = 0; i < num_modes; i++) { + if (i > 0 && modes[i - 1].mode > modes[i].mode) { + LOGP(DRR, LOGL_ERROR, +"BUG: Multirate codec with inconsistant config (mode order).\n"); + return -EINVAL; + } + if (i > 0 && modes[i - 1].mode == modes[i].mode) { + LOGP(DRR, LOGL_ERROR, +"BUG: Multirate codec with inconsistant config (duplicate modes).\n"); + return -EINVAL; + } + } + + /* Check if the active set that is defined in mr_conf has at least one +* mode but not more than 4 modes set */ for (i = 0; i < 8; i++) { - if (((mr->gsm48_ie[1] >> i) & 1)) + if (((gsm48_ie[1] >> i) & 1)) num++; } if (num > 4) { - LOGP(DRR, LOGL_ERROR, "BUG: Using multirate codec with too " - "many modes in config.\n"); - num = 4; + LOGP(DRR, LOGL_ERROR, +"BUG: Multirate codec with too many modes in config.\n"); + return -EINVAL; } if (num < 1) { - LOGP(DRR, LOGL_ERROR, "BUG: Using m