Change in osmo-bsc[master]: gsm_04_08: improve gsm48_multirate_config()

2018-10-24 Thread dexter 
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.ms_mode);
-   gsm48_multirate_config(lchan->mr_bts_lv, , 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()

2018-10-23 Thread Harald Welte 
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()

2018-10-23 Thread dexter 
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()

2018-10-23 Thread dexter 
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