[PATCH] libosmocore[master]: VTY: implicit node exit by de-indenting, not parent lookup

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3880

to look at the new patch set (#2).

VTY: implicit node exit by de-indenting, not parent lookup

Note: This will break users' config files if they do not use consistent
indenting. (see below for a definition of "consistent".)

When reading VTY commands from a file, use indenting as means to implicitly
exit child nodes. Do not look for commands in the parent node implicitly.

The VTY so far implies 'exit' commands if a VTY line cannot be parsed on the
current node, but succeeds on the parent node. That is the mechanism by which
our VTY config files do not need 'exit' at the end of each child node.

We've hit problems with this in the following scenarios, which will show
improved user experience after this patch:

*) When both a parent and its child node have commands with identical names:

  cs7 instace 0
   point-code 1.2.3
   sccp-address osmo-msc
point-code 0.0.1

If I put the parent's command below the child, it is still interpreted in the
context of the child node:

  cs7 instace 0
   sccp-address osmo-msc
point-code 0.0.1
   point-code 1.2.3

Though the indenting lets me assume I am setting the cs7 instance's global PC
to 1.2.3, I'm actually overwriting osmo-msc's PC with 1.2.3 and discarding the
0.0.1.

*) When a software change moves a VTY command from a child to a parent. Say
'timezone' moved from 'bts' to 'network' level:

  network
   timezone 1 2

Say a user still has an old config file with 'timezone' on the child level:

  network
   bts 0
timezone 1 2
trx 0

The user would expect an error message that 'timezone' is invalid on the 'bts'
level. Instead, the VTY finds the parent node's 'timezone', steps out of 'bts'
to the 'network' level, and instead says that the 'trx' command does not exist.

Format:

Consistent means that two adjacent indenting lines have the exact
same indenting characters for the common length:

Weird mix if you ask me, but correct and consistent:

  ROOT
  PARENT
  CHILD
  GRANDCHILD
  GRANDCHILD2
  SIBLING

Inconsistent:

  ROOT
  PARENT
  CHILD
  GRANDCHILD
  GRANDCHILD2
  SIBLING

Also, when going back to a parent level, the exact same indenting must be used
as before in that node:

Incorrect:

  ROOT
  PARENT
  CHILD
  SIBLING

As not really intended side effect, it is also permitted to indent the entire
file starting from the root level. We could guard against it but there's no
harm:

Correct and consistent:

  ROOT
  PARENT
  CHILD
  SIBLING

Implementation:

Track parent nodes state: whenever a command enters a child node, push a parent
node onto an llist to remember the exact indentation characters used for that
level.

As soon as the first line on a child node is parsed, remember this new
indentation (which must have a longer strlen() than its parent level) to apply
to all remaining child siblings and grandchildren.

If the amount of spaces that indent a following VTY command are less than this
expected indentation, call vty_go_parent() until it matches up.

At any level, if the common length of indentation characters mismatch, abort
parsing in error.

Transitions to child node are spread across VTY implementations and are hard to
change. But transitions to the parent node are all handled by vty_go_parent().
By popping a parent from the list of parents in vty_go_parent(), we can also
detect that a command has changed the node without changing the parent, hence
it must have stepped into a child node, and we can push a parent frame.

The behavior on the interactive telnet VTY remains unchanged.

Change-Id: I24cbb3f6de111f2d31110c3c484c066f1153aac9
---
M include/osmocom/vty/command.h
M include/osmocom/vty/vty.h
M src/vty/command.c
M src/vty/vty.c
M tests/Makefile.am
M tests/testsuite.at
A tests/vty/fail_not_de-indented.cfg
A tests/vty/fail_tabs_and_spaces.cfg
A tests/vty/fail_too_much_indent.cfg
A tests/vty/ok.cfg
A tests/vty/ok_ignore_blank.cfg
A tests/vty/ok_ignore_comment.cfg
A tests/vty/ok_indented_root.cfg
A tests/vty/ok_more_spaces.cfg
A tests/vty/ok_tabs.cfg
A tests/vty/ok_tabs_and_spaces.cfg
M tests/vty/vty_test.c
M tests/vty/vty_test.ok
18 files changed, 312 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/80/3880/2

diff --git a/include/osmocom/vty/command.h b/include/osmocom/vty/command.h
index 0fa5175..cb2edaa 100644
--- a/include/osmocom/vty/command.h
+++ b/include/osmocom/vty/command.h
@@ -161,6 +161,7 @@
 #define CMD_COMPLETE_MATCH   8
 #define CMD_COMPLETE_LIST_MATCH  9
 #define CMD_SUCCESS_DAEMON  10
+#define CMD_ERR_INVALID_INDENT  11
 
 /* Argc max counts. */
 #define CMD_ARGC_MAX   256
@@ -368,6 +369,7 @@
 char *argv_concat(const char **argv, int argc, int shift);
 
 vector cmd_make_strvec(const char *);
+int cmd_make_strvec2(const char *string, char **indent, vector *strvec_p);
 void cmd_free_strvec(vector);
 vector cmd_describe_command();
 char 

[PATCH] libosmocore[master]: VTY: allow comments in the same line as vty commands

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3882

to look at the new patch set (#2).

VTY: allow comments in the same line as vty commands

Note: this breaks users' VTY config when a command's argument starts with a '!'
or '#'.

Allow comments following VTY commands, on the same line. Start a comment with
'!' or '#' following a space, which allows using the comment characters in
command arguments -- just not at the start of an argument.

This patch allows to do:

   remote-ip 10.23.24.1# where to reach the STP

When they are not preceded by a space, the comment characters still count as
VTY command tokens, i.e. still allowing:

   short-name instance#23

This would parse "instance#23" as name.

(We usually use '!' as the VTY comment delimiter, but the code also allowed and
still allows using '#', hence add both '#' and '!' as same-line comment
delimiter.)

Change-Id: Iccd9cc8604494379910c534b35ce7e74e329d863
---
M src/vty/command.c
M tests/Makefile.am
A tests/vty/ok_comments.cfg
M tests/vty/vty_test.c
M tests/vty/vty_test.ok
5 files changed, 11 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/82/3882/2

diff --git a/src/vty/command.c b/src/vty/command.c
index a65b4de..0f5538a 100644
--- a/src/vty/command.c
+++ b/src/vty/command.c
@@ -246,6 +246,9 @@
 
/* Copy each command piece and set into vector. */
while (1) {
+   if (*cp == '!' || *cp == '#')
+   break;
+
start = cp;
while (!(isspace((int)*cp) || *cp == '\r' || *cp == '\n') &&
   *cp != '\0')
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8526847..5a124c1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -218,7 +218,8 @@
  fr/fr_test.ok loggingrb/logging_test.ok   \
  loggingrb/logging_test.errstrrb/strrb_test.ok 
\
 vty/vty_test.ok vty/fail_not_de-indented.cfg   \
-vty/fail_too_much_indent.cfg vty/ok.cfg\
+vty/fail_too_much_indent.cfg vty/ok.cfg\
+vty/ok_comments.cfg\
 comp128/comp128_test.ok\
 utils/utils_test.ok stats/stats_test.ok\
 bitvec/bitvec_test.ok msgb/msgb_test.ok bits/bitcomp_test.ok \
diff --git a/tests/vty/ok_comments.cfg b/tests/vty/ok_comments.cfg
new file mode 100644
index 000..c040892
--- /dev/null
+++ b/tests/vty/ok_comments.cfg
@@ -0,0 +1,3 @@
+line vty ! comments
+ no login # after
+log stderr #commands
diff --git a/tests/vty/vty_test.c b/tests/vty/vty_test.c
index eba9995..27e5b5c 100644
--- a/tests/vty/vty_test.c
+++ b/tests/vty/vty_test.c
@@ -342,6 +342,7 @@
test_exit_by_indent("fail_too_much_indent.cfg", -EINVAL);
test_exit_by_indent("fail_tabs_and_spaces.cfg", -EINVAL);
test_exit_by_indent("ok_indented_root.cfg", 0);
+   test_exit_by_indent("ok_comments.cfg", 0);
 
/* Leak check */
OSMO_ASSERT(talloc_total_blocks(stats_ctx) == 1);
diff --git a/tests/vty/vty_test.ok b/tests/vty/vty_test.ok
index b2df1a1..a09e7ef 100644
--- a/tests/vty/vty_test.ok
+++ b/tests/vty/vty_test.ok
@@ -128,4 +128,6 @@
 got rc=-22
 reading file ok_indented_root.cfg, expecting rc=0
 got rc=0
+reading file ok_comments.cfg, expecting rc=0
+got rc=0
 All tests passed

-- 
To view, visit https://gerrit.osmocom.org/3882
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iccd9cc8604494379910c534b35ce7e74e329d863
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 

[PATCH] libosmocore[master]: osmo-auc-gen.c: squelch compiler warnings, move local var

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/3908

osmo-auc-gen.c: squelch compiler warnings, move local var

The compiler thinks that ind or ind_mask may be used uninitialized, because it
doesn't analyze the conditionality of command line arguments and other
variables set accordingly. Make the compiler happy by zero initializing.

Change-Id: I9ddcb0525159da520aceaeb6e908a735a003bb5a
---
M utils/osmo-auc-gen.c
1 file changed, 3 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/08/3908/1

diff --git a/utils/osmo-auc-gen.c b/utils/osmo-auc-gen.c
index fee9767..1f5c838 100644
--- a/utils/osmo-auc-gen.c
+++ b/utils/osmo-auc-gen.c
@@ -98,15 +98,14 @@
struct osmo_auth_vector *vec = &_vec;
uint8_t _rand[16], _auts[14];
uint64_t sqn;
-   unsigned int ind;
+   unsigned int ind = 0;
int rc, option_index;
int rand_is_set = 0;
int auts_is_set = 0;
int sqn_is_set = 0;
int ind_is_set = 0;
int fmt_triplets_dat = 0;
-   uint64_t seq_1;
-   uint64_t ind_mask;
+   uint64_t ind_mask = 0;
 
printf("osmo-auc-gen (C) 2011-2012 by Harald Welte\n");
printf("This is FREE SOFTWARE with ABSOLUTELY NO WARRANTY\n\n");
@@ -270,7 +269,7 @@
memset(vec, 0, sizeof(*vec));
 
if (test_aud.type == OSMO_AUTH_TYPE_UMTS) {
-   seq_1 = 1LL << test_aud.u.umts.ind_bitlen;
+   uint64_t seq_1 = 1LL << test_aud.u.umts.ind_bitlen;
ind_mask = seq_1 - 1;
 
if (sqn_is_set) {

-- 
To view, visit https://gerrit.osmocom.org/3908
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9ddcb0525159da520aceaeb6e908a735a003bb5a
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 

libosmo-netif[master]: src: _snprintf() helper functions always nul-terminate buffers

[Pau Espin Pedrol]

Patch Set 1:

> OK.
 > 
 > So these are the two cases that we could improve:
 > 
 > 1) snprintf() returns -1, very much unlikely in practise.
 > 2) msg->len == 0: In such case, I would expect this function is
 > never called with an empty message...
 > 
 > Frankly speaking, is there any realistic setup on your side that
 > you believe may bite the dust because of these two overly corner
 > cases?

That's all I could find. If you want to spend time solving those, the go for 
it, otherwise it can already be merged as in any case it's better that what we 
currently have. But sticking to those cases and specs I'm just stating that you 
could get a crash by using this function.

-- 
To view, visit https://gerrit.osmocom.org/3830
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I97e517f2d98e83894ea707c63489559302ff6bd2
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

openbsc[master]: libmsc: sms_route_mt_sms() may return uninitialized return v...

[Pablo Neira Ayuso]

Patch Set 2:

Hm, Jenkins is complaining on VTY tests again. This patch doesn't update such 
bits. Any clue on what is wrong?

Thanks!

-- 
To view, visit https://gerrit.osmocom.org/3899
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c0bcd919cc3275d491995d17c6a32bb61c6afe1
Gerrit-PatchSet: 2
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Pablo Neira Ayuso]

Patch Set 4: Verified+1

-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 4
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

[MERGED] openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Pablo Neira Ayuso]

Pablo Neira Ayuso has submitted this change and it was merged.

Change subject: libmsc: annotate esme route in the sms object from 
deliver_to_esme()
..


libmsc: annotate esme route in the sms object from deliver_to_esme()

Annotate this esme route, so we can use it to return -EINPROGRESS to
skip sending premature RP-ACK to the mobile station, in case we're
handling sms routes through SMPP.

Now that we have this information in place, we use it wherever possible
to avoid kludgy checks on sms->receiver.

sms_free() already releases references to this object, so we should be
fine with this.

Fixes: 4e5b90a594f9 ("libmsc: remove 'deferred' parameter in 
sms_route_mt_sms()")
Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
---
M openbsc/src/libmsc/gsm_04_11.c
M openbsc/src/libmsc/smpp_openbsc.c
2 files changed, 12 insertions(+), 3 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Pablo Neira Ayuso: Verified



diff --git a/openbsc/src/libmsc/gsm_04_11.c b/openbsc/src/libmsc/gsm_04_11.c
index ddef444..27bffc9 100644
--- a/openbsc/src/libmsc/gsm_04_11.c
+++ b/openbsc/src/libmsc/gsm_04_11.c
@@ -519,7 +519,10 @@
 
rc = sms_route_mt_sms(conn, gsms);
 
-   /* This SMS got routed through SMPP or no receiver exists. */
+   /* This SMS got routed through SMPP. */
+   if (gsms->smpp.esme)
+   return -EINPROGRESS;
+
if (!gsms->receiver)
return rc;
 
@@ -613,8 +616,10 @@
return gsm411_send_rp_ack(trans, rph->msg_ref);
else if (rc > 0)
return gsm411_send_rp_error(trans, rph->msg_ref, rc);
-   else
-   return rc;
+   else if (rc == -EINPROGRESS)
+   rc = 0;
+
+   return rc;
 }
 
 /* Receive a 04.11 RP-DATA message in accordance with Section 7.3.1.2 */
diff --git a/openbsc/src/libmsc/smpp_openbsc.c 
b/openbsc/src/libmsc/smpp_openbsc.c
index af2d1be..3fe2dfd 100644
--- a/openbsc/src/libmsc/smpp_openbsc.c
+++ b/openbsc/src/libmsc/smpp_openbsc.c
@@ -714,6 +714,10 @@
if (ret < 0)
return ret;
 
+   OSMO_ASSERT(!sms->smpp.esme);
+   smpp_esme_get(esme);
+   sms->smpp.esme = esme;
+
return smpp_cmd_enqueue(esme, conn->subscr, sms,
deliver.sequence_number);
 }

-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 4
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: neels 

[PATCH] openbsc[master]: libmsc: sms_route_mt_sms() may return uninitialized return v...

[Pablo Neira Ayuso]

Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3899

to look at the new patch set (#2).

libmsc: sms_route_mt_sms() may return uninitialized return value

If smpp_first is set off and there is a local receiver for this sms,
then return 0.

Without this patch, we return 'rc' which is uninitialized in the
scenario that I'm describing above.

Change-Id: I0c0bcd919cc3275d491995d17c6a32bb61c6afe1
---
M openbsc/src/libmsc/gsm_04_11.c
1 file changed, 22 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/99/3899/2

diff --git a/openbsc/src/libmsc/gsm_04_11.c b/openbsc/src/libmsc/gsm_04_11.c
index ddef444..eb0a8ae 100644
--- a/openbsc/src/libmsc/gsm_04_11.c
+++ b/openbsc/src/libmsc/gsm_04_11.c
@@ -363,31 +363,32 @@
/* determine gsms->receiver based on dialled number */
gsms->receiver = subscr_get_by_extension(conn->network->subscr_group,
 gsms->dst.addr);
-   if (!gsms->receiver) {
-#ifdef BUILD_SMPP
-   /* Avoid a second look-up */
-   if (smpp_first) {
-   
rate_ctr_inc(>network->msc_ctrs->ctr[MSC_CTR_SMS_NO_RECEIVER]);
-   return GSM411_RP_CAUSE_MO_NUM_UNASSIGNED;
-   }
+   if (gsms->receiver)
+   return 0;
 
-   rc = smpp_try_deliver(gsms, conn);
-   if (rc == GSM411_RP_CAUSE_MO_NUM_UNASSIGNED) {
-   
rate_ctr_inc(>network->msc_ctrs->ctr[MSC_CTR_SMS_NO_RECEIVER]);
-   } else if (rc < 0) {
-   LOGP(DLSMS, LOGL_ERROR, "%s: SMS delivery error: %d.",
-subscr_name(conn->subscr), rc);
-   rc = GSM411_RP_CAUSE_MO_TEMP_FAIL;
-   /* rc will be logged by gsm411_send_rp_error() */
-   rate_ctr_inc(>bts->network->msc_ctrs->ctr[
-   MSC_CTR_SMS_DELIVER_UNKNOWN_ERROR]);
-   }
-#else
-   rc = GSM411_RP_CAUSE_MO_NUM_UNASSIGNED;
+#ifdef BUILD_SMPP
+   /* Avoid a second look-up */
+   if (smpp_first) {

rate_ctr_inc(>network->msc_ctrs->ctr[MSC_CTR_SMS_NO_RECEIVER]);
-#endif
+   return GSM411_RP_CAUSE_MO_NUM_UNASSIGNED;
}
 
+   rc = smpp_try_deliver(gsms, conn);
+   if (rc == GSM411_RP_CAUSE_MO_NUM_UNASSIGNED) {
+   
rate_ctr_inc(>network->msc_ctrs->ctr[MSC_CTR_SMS_NO_RECEIVER]);
+   } else if (rc < 0) {
+   LOGP(DLSMS, LOGL_ERROR, "%s: SMS delivery error: %d.",
+subscr_name(conn->subscr), rc);
+   rc = GSM411_RP_CAUSE_MO_TEMP_FAIL;
+   /* rc will be logged by gsm411_send_rp_error() */
+   rate_ctr_inc(>bts->network->msc_ctrs->ctr[
+   MSC_CTR_SMS_DELIVER_UNKNOWN_ERROR]);
+   }
+#else
+   rc = GSM411_RP_CAUSE_MO_NUM_UNASSIGNED;
+   rate_ctr_inc(>network->msc_ctrs->ctr[MSC_CTR_SMS_NO_RECEIVER]);
+#endif
+
return rc;
 }
 

-- 
To view, visit https://gerrit.osmocom.org/3899
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0c0bcd919cc3275d491995d17c6a32bb61c6afe1
Gerrit-PatchSet: 2
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 

openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Harald Welte]

Patch Set 4: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 4
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

[MERGED] libosmo-netif[master]: osmux: fix buffer management mess in snprintf() calls

[Pablo Neira Ayuso]

Pablo Neira Ayuso has submitted this change and it was merged.

Change subject: osmux: fix buffer management mess in snprintf() calls
..


osmux: fix buffer management mess in snprintf() calls

SNPRINTF_BUFFER_SIZE() looks too complex, previous version maintains two
different variables to account for the remaining space in the buffer,
one of them is always decremented based on what snprintf() returns,
which may result in underflow. These variables are swapped - not used
consistently - all over this code.

Replace this macro by a simplified version, with one single parameter to
account for remaining space. This macro also deals with two corner
cases:

1) snprintf() fails, actually never happens in practise, but
   documentation indicates it may return -1, so let's catch this case
   from here to stick to specs.

2) There is not enough space in the buffer, in that case, keep
   increasing offset, so we know how much would have been printed, just
   like snprintf() does.

Thanks to Pau Espin for reporting, and Holger for clues on this.
I have run osmux_test and, at quick glance, it looks good.

Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
---
M src/osmux.c
M src/rtp.c
M tests/osmux/osmux_test.c
3 files changed, 44 insertions(+), 45 deletions(-)

Approvals:
  Pau Espin Pedrol: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/osmux.c b/src/osmux.c
index b3c43e2..b18246f 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -846,19 +846,20 @@
h->rtp_ssrc = rtp_ssrc;
 }
 
-#define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)   \
-   size -= ret;\
-   if (ret > len)  \
-   ret = len;  \
+#define SNPRINTF_BUFFER_SIZE(ret, remain, offset)  \
+   if (ret < 0)\
+   ret = 0;\
offset += ret;  \
-   len -= ret;
+   if (ret > remain)   \
+   ret = remain;   \
+   remain -= ret;
 
 static int osmux_snprintf_header(char *buf, size_t size, struct osmux_hdr 
*osmuxh)
 {
+   unsigned int remain = size, offset = 0;
int ret;
-   int len = size, offset = 0;
 
-   ret = snprintf(buf, len, "OSMUX seq=%03u ccid=%03u "
+   ret = snprintf(buf, remain, "OSMUX seq=%03u ccid=%03u "
 "ft=%01u ctr=%01u "
 "amr_f=%01u amr_q=%01u "
 "amr_ft=%02u amr_cmr=%02u ",
@@ -866,7 +867,7 @@
osmuxh->ft, osmuxh->ctr,
osmuxh->amr_f, osmuxh->amr_q,
osmuxh->amr_ft, osmuxh->amr_cmr);
-   SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+   SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
return offset;
 }
@@ -874,19 +875,19 @@
 static int osmux_snprintf_payload(char *buf, size_t size,
  const uint8_t *payload, int payload_len)
 {
+   unsigned int remain = size, offset = 0;
int ret, i;
-   int len = size, offset = 0;
 
-   ret = snprintf(buf+offset, len, "[ ");
-   SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+   ret = snprintf(buf + offset, remain, "[ ");
+   SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
for (i=0; ilen, len = size;
-   struct osmux_hdr *osmuxh;
+   unsigned int remain = size;
int this_len, msg_off = 0;
+   struct osmux_hdr *osmuxh;
+   unsigned int offset = 0;
+   int msg_len = msg->len;
+   int ret;
 
while (msg_len > 0) {
if (msg_len < sizeof(struct osmux_hdr)) {
@@ -915,10 +917,8 @@
return -1;
}
 
-   ret = osmux_snprintf_header(buf+offset, size, osmuxh);
-   if (ret < 0)
-   break;
-   SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+   ret = osmux_snprintf_header(buf + offset, remain, osmuxh);
+   SNPRINTF_BUFFER_SIZE(ret, remain, 

[ABANDON] openbsc[master]: libmsc: Either route report to ESME or send it, not both

[Keith Whyte]

Keith Whyte has abandoned this change.

Change subject: libmsc: Either route report to ESME or send it, not both
..


Abandoned

https://gerrit.osmocom.org/#/c/3899/
is dealing with this correctly

-- 
To view, visit https://gerrit.osmocom.org/3885
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I8e86a34f5d3087c9c25479192d9a690922113da2
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Keith Whyte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pablo Neira Ayuso 

osmo-pcu[master]: Simplify TS alloc: adjust function signatures

[Harald Welte]

Patch Set 5: Code-Review+1

-- 
To view, visit https://gerrit.osmocom.org/3807
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I39d81ab64ff790b9c4c2d0312a574485cd83e755
Gerrit-PatchSet: 5
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: No

osmo-pcu[master]: TS alloc: update tests

[Harald Welte]

Patch Set 8: Code-Review+1

-- 
To view, visit https://gerrit.osmocom.org/3895
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4b78951a79ddbc0745b39d091080a4e0e247d3c5
Gerrit-PatchSet: 8
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-HasComments: No

libosmo-netif[master]: osmux: fix buffer management mess in snprintf() calls

[Harald Welte]

Patch Set 4:

> BTW, it seems Jenkins is experiencing problems? 
For some reason, our primary build host was down due to repeated kernel oopses 
earlier today.  I've rebooted it, it's working again. You need to cherry-pick 
the patch to trigger a re-build (which i just did)

-- 
To view, visit https://gerrit.osmocom.org/3825
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 4
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Harald Welte]

Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/3900/1/openbsc/src/libmsc/smpp_openbsc.c
File openbsc/src/libmsc/smpp_openbsc.c:

Line 718:   sms->smpp.esme = esme;
> Please, if you carefully review the codepath you will notice that is always
Pablo, please calm down. We're all friends here.  I agree with neels that it is 
not obvious in the current code that the callers never have set sms->smpp.esme 
before, particularly as the sms pointer is passed in by callers.  Any later 
updates / extension of the code might introduce subbtle refcount leaks here,  
At least, I would argue for a OSMO_ASSERT(!sms->smpp.esme) before the 
assignment.  This way, if caller code ever changes, we get a proper failure 
rather than a refcount leak.


-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: neels 
Gerrit-HasComments: Yes

libosmo-netif[master]: osmux: fix buffer management mess in snprintf() calls

[Harald Welte]

Patch Set 4: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3825
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 4
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

[PATCH] osmo-pcu[master]: Simplify TS alloc: split allocation

[Max]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3906

to look at the new patch set (#3).

Simplify TS alloc: split allocation

* generalize TS allocation and move it into separate function
* move single-slot allocation into separate function
* use common functions for TS allocation on both UL and DL

Change-Id: Ied45ae380c345bc76fe9d6fd9a6184d1109f83f2
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 103 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/06/3906/3

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index bcac45f..3d490c9 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -882,6 +882,95 @@
}
 }
 
+/*! Count used bits in slots and reserved_slots bitmasks
+ *
+ *  \param[in] slots Timeslots in use
+ *  \param[in] reserved_slots Reserved timeslots
+ *  \param[out] slotcount Number of TS in use
+ *  \param[out] avail_count Number of reserved TS
+ */
+static void update_slot_counters(uint8_t slots, uint8_t reserved_slots, 
uint8_t *slotcount, uint8_t *avail_count)
+{
+   (*slotcount) = pcu_bitcount(slots);
+   (*avail_count) = pcu_bitcount(reserved_slots);
+}
+
+/*! Return slot mask with single TS from a given UL/DL set according to TBF's 
direction, ts pointer is set to that TS
+ * number or to negative value on error
+ *
+ *  \param[in] trx Pointer to TRX object
+ *  \param[in] tbf Pointer to TBF object
+ *  \param[in] dl_slots set of DL timeslots
+ *  \param[in] ul_slots set of UL timeslots
+ *  \param[in,out] ts corresponding TS or -1 for autoselection
+ *  \returns slot mask with single UL or DL timeslot number if possible
+ */
+static uint8_t get_single_ts(const gprs_rlcmac_trx *trx, const gprs_rlcmac_tbf 
*tbf, uint8_t dl_slots, uint8_t ul_slots,
+int *ts)
+{
+   uint8_t ret = dl_slots & ul_slots; /* Make sure to consider the first 
common slot only */
+
+   if (*ts < 0)
+   *ts = find_least_busy_pdch(trx, tbf->direction, ret, 
compute_usage_by_num_tbfs, NULL, NULL);
+
+   if (*ts < 0)
+   return ffs(ret);
+
+   return ret & (1 << (*ts));
+}
+
+/*! Find set of timeslots available for allocation
+ *
+ *  \param[in] trx Pointer to TRX object
+ *  \param[in] tbf Pointer to TBF object
+ *  \param[in] single Flag to force the single TS allocation
+ *  \param[in] ul_slots set of UL timeslots
+ *  \param[in] ul_slots set of DL timeslots
+ *  \param[in] reserved_ul_slots set of reserved UL timeslots
+ *  \param[in] reserved_dl_slots set of reserved DL timeslots
+ *  \param[in] first_common_ts First TS common for both UL and DL or -1 if 
unknown
+ *  \returns negative error code or selected TS on success
+ */
+static int tbf_select_slot_set(const gprs_rlcmac_tbf *tbf, const 
gprs_rlcmac_trx *trx, bool single,
+  uint8_t ul_slots, uint8_t dl_slots,
+  uint8_t reserved_ul_slots, uint8_t 
reserved_dl_slots,
+  int8_t first_common_ts)
+{
+   uint8_t sl, res;
+   int ts = first_common_ts;
+   char *dir, *abrev, slot_info[9] = { 0 }, small, big;
+
+   if (tbf->direction != GPRS_RLCMAC_DL_TBF) {
+   sl = ul_slots;
+   res = reserved_ul_slots;
+   small = 'u';
+   big = 'U';
+   dir = (char *)"uplink";
+   abrev = (char *)"UL";
+   } else {
+   sl = dl_slots;
+   res = reserved_dl_slots;
+   small = 'd';
+   big = 'D';
+   dir = (char *)"downlink";
+   abrev = (char *)"DL";
+   }
+
+   if (single)
+   sl = get_single_ts(trx, tbf, dl_slots, ul_slots, );
+
+   if (sl == 0) {
+   LOGP(DRLCMAC, LOGL_NOTICE, "No %s slots available\n", dir);
+   return -EINVAL;
+   }
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Selected %s slots: 
(TS=0)\"%s\"(TS=7)%s\n", abrev,
+set_flag_chars(set_flag_chars(slot_info, res, small, '.'), sl, 
big),
+single ? ", single" : "");
+
+   return sl;
+}
+
 /*! Slot Allocation: Algorithm B
  *
  * Assign as many downlink slots as possible.
@@ -904,7 +993,6 @@
int8_t first_common_ts;
uint8_t slotcount = 0;
uint8_t avail_count = 0, trx_no;
-   char slot_info[9] = {0};
int ts;
int first_ts = -1;
int usf[8] = {-1, -1, -1, -1, -1, -1, -1, -1};
@@ -946,86 +1034,43 @@
reserved_dl_slots = dl_slots;
reserved_ul_slots = ul_slots;
 
-   /* Step 3: Derive the slot set for the current TBF */
-   if (single) {
-   /* Make sure to consider the first common slot only */
-   ul_slots = dl_slots = dl_slots & ul_slots;
-
-   ts = first_common_ts;
-
-   if (ts < 0)
-   ts = 

[PATCH] osmo-pcu[master]: TS alloc: update tests

[Max]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3895

to look at the new patch set (#8).

TS alloc: update tests

* restructure code for easier reading
* rearrange tests order to facilitate further UL alloc changes
* use consistent formatting for output
* log essential allocation parameters on failure

Changes to tests are combined with logging messages update to separate
code and test changes over different commits to avoid regressions.

Change-Id: I4b78951a79ddbc0745b39d091080a4e0e247d3c5
Related: OS#2282
---
M src/bts.cpp
M src/gprs_rlcmac_ts_alloc.cpp
M tests/alloc/AllocTest.cpp
M tests/alloc/AllocTest.err
M tests/alloc/AllocTest.ok
M tests/tbf/TbfTest.err
6 files changed, 3,287 insertions(+), 3,289 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/95/3895/8


-- 
To view, visit https://gerrit.osmocom.org/3895
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b78951a79ddbc0745b39d091080a4e0e247d3c5
Gerrit-PatchSet: 8
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 

libosmo-netif[master]: osmux: fix buffer management mess in snprintf() calls

[Pablo Neira Ayuso]

Patch Set 3:

Thanks for reviewing Pau!

BTW, it seems Jenkins is experiencing problems? By reading the console output 
it looks the build failed for reasons other than this patch.

-- 
To view, visit https://gerrit.osmocom.org/3825
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 3
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Pablo Neira Ayuso]

Patch Set 1: Code-Review+1

(1 comment)

https://gerrit.osmocom.org/#/c/3900/1/openbsc/src/libmsc/smpp_openbsc.c
File openbsc/src/libmsc/smpp_openbsc.c:

Line 718:   sms->smpp.esme = esme;
> check that we're not overwriting sms->smpp.esme if it is already set? (i.e 
Please, if you carefully review the codepath you will notice that is always 
unset...

However, you vote this with -1 and this is blocking a real fix, something that 
Keith confirms that is solving a real problem there.

I don't want to add this superfluous check, it's better to agree on semantics, 
not just add a branch there because maybe someone else make a sloppy use of 
this in the future...

Stop forcing people to do overly defensive programming, just because you are 
scared.

Or develop your real concerns, instead of nacking work from others with poor 
arguments.

Thank you.


-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pablo Neira Ayuso 
Gerrit-Reviewer: neels 
Gerrit-HasComments: Yes

[MERGED] libosmo-sccp[master]: vty: add 'asp' / 'local-ip' command

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: vty: add 'asp' / 'local-ip' command
..


vty: add 'asp' / 'local-ip' command

We can set the ASP's remote IP (i.e. where to reach osmo-stp), but so far the
only way to specify the local IP address to bind to can only be set from C code
(e.g. the simple client). Allow setting the local address via VTY.

For example, this is desired for the osmo-gsm-tester, to not use arbitrary IP
addresses.

Change-Id: I3f71897dfacafcf3126e51894d6ca756b02dcd7d
---
M src/osmo_ss7_vty.c
1 file changed, 13 insertions(+), 0 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/osmo_ss7_vty.c b/src/osmo_ss7_vty.c
index 1fb35a1..8d13865 100644
--- a/src/osmo_ss7_vty.c
+++ b/src/osmo_ss7_vty.c
@@ -556,6 +556,16 @@
return CMD_SUCCESS;
 }
 
+DEFUN(asp_local_ip, asp_local_ip_cmd,
+   "local-ip A.B.C.D",
+   "Specify Local IP Address from which to contact ASP\n"
+   "Local IP Address from which to contact of ASP\n")
+{
+   struct osmo_ss7_asp *asp = vty->index;
+   osmo_talloc_replace_string(asp, >cfg.local.host, argv[0]);
+   return CMD_SUCCESS;
+}
+
 DEFUN(asp_remote_ip, asp_remote_ip_cmd,
"remote-ip A.B.C.D",
"Specify Remote IP Address of ASP\n"
@@ -633,6 +643,8 @@
osmo_ss7_asp_protocol_name(asp->cfg.proto), VTY_NEWLINE);
if (asp->cfg.description)
vty_out(vty, "  description %s%s", asp->cfg.description, 
VTY_NEWLINE);
+   if (asp->cfg.local.host)
+   vty_out(vty, "  local-ip %s%s", asp->cfg.local.host, 
VTY_NEWLINE);
if (asp->cfg.remote.host)
vty_out(vty, "  remote-ip %s%s", asp->cfg.remote.host, 
VTY_NEWLINE);
if (asp->cfg.qos_class)
@@ -1728,6 +1740,7 @@
install_element(L_CS7_NODE, _cs7_asp_cmd);
install_element(L_CS7_ASP_NODE, _description_cmd);
install_element(L_CS7_ASP_NODE, _remote_ip_cmd);
+   install_element(L_CS7_ASP_NODE, _local_ip_cmd);
install_element(L_CS7_ASP_NODE, _qos_class_cmd);
install_element(L_CS7_ASP_NODE, _block_cmd);
install_element(L_CS7_ASP_NODE, _shutdown_cmd);

-- 
To view, visit https://gerrit.osmocom.org/3826
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3f71897dfacafcf3126e51894d6ca756b02dcd7d
Gerrit-PatchSet: 2
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 

osmo-gsm-tester[master]: modem: Fix race condition when connect() is called more than...

[Neels Hofmeyr]

Patch Set 2:

We can already grep osmo-msc stderr to assert no accept and one reject, without 
adding external dependencies. But in general if we're adding pcap grepping 
anyway, we might as well grep the pcap here, too.

-- 
To view, visit https://gerrit.osmocom.org/3901
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4bb1c6b72c23cd860c33bee7851bca3d0ac0e1b
Gerrit-PatchSet: 2
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

[PATCH] osmo-pcu[master]: Simplify TS alloc: re-arrange code

[Max]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3760

to look at the new patch set (#10).

Simplify TS alloc: re-arrange code

* consistently format log messages to make it possible to grep for test
output in source code
* move TRX check inside local tfi_find_free() wrapper
* assign reserved_*_slots only when multislot masks are found

Change-Id: I02da2b8ba8c9c8815dae0e39e1fed277ca0df171
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 21 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/60/3760/10

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index c5b398f..bcac45f 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -248,9 +248,7 @@
if (free_tfi) {
tfi = find_free_tfi(pdch, dir);
if (tfi < 0) {
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "no TFI available\n", ts);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping 
TS %d, because no TFI available\n", ts);
continue;
}
}
@@ -258,26 +256,20 @@
if (dir == GPRS_RLCMAC_UL_TBF) {
usf = find_free_usf(pdch);
if (usf < 0) {
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "no USF available\n", ts);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping 
TS %d, because no USF available\n", ts);
continue;
}
}
if (min_ts >= 0)
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "num TBFs %d > %d\n",
-   min_ts, min_used, num_tbfs);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping TS %d, 
because num TBFs %d > %d\n",
+min_ts, min_used, num_tbfs);
min_used = num_tbfs;
min_ts = ts;
min_tfi = tfi;
min_usf = usf;
} else {
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "num TBFs %d >= %d\n",
-   ts, num_tbfs, min_used);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping TS %d, because 
num TBFs %d >= %d\n",
+ts, num_tbfs, min_used);
}
}
 
@@ -374,7 +366,7 @@
 /*! Return free TFI
  *
  *  \param[in] bts Pointer to BTS struct
- *  \param[in] trx Pointer to TRX struct
+ *  \param[in] trx Optional pointer to TRX struct
  *  \param[in] ms Pointer to MS object
  *  \param[in] dir DL or UL direction
  *  \param[in] use_trx which TRX to use or -1 if it should be selected based 
on what MS uses
@@ -386,6 +378,15 @@
 {
int tfi;
uint8_t trx_no;
+
+   if (trx) {
+   if (use_trx >= 0 && use_trx != trx->trx_no) {
+   LOGP(DRLCMAC, LOGL_ERROR, "- Requested incompatible TRX 
%d (current is %d)\n",
+use_trx, trx->trx_no);
+   return -EINVAL;
+   }
+   use_trx = trx->trx_no;
+   }
 
if (use_trx == -1 && ms->current_trx())
use_trx = ms->current_trx()->trx_no;
@@ -920,20 +921,10 @@
return -EINVAL;
}
 
-   reserved_dl_slots = dl_slots = ms->reserved_dl_slots();
-   reserved_ul_slots = ul_slots = ms->reserved_ul_slots();
+   dl_slots = ms->reserved_dl_slots();
+   ul_slots = ms->reserved_ul_slots();
first_common_ts = ms->first_common_ts();
trx = ms->current_trx();
-
-   if (trx) {
-   if (use_trx >= 0 && use_trx != trx->trx_no) {
-   LOGP(DRLCMAC, LOGL_ERROR,
-   "- Requested incompatible TRX %d (current is 
%d)\n",
-   use_trx, trx->trx_no);
-   return -EINVAL;
-   }
-   use_trx = trx->trx_no;
-   }
 
/* Step 2a: Find usable TRX and TFI */
tfi = tfi_find_free(bts->bts, trx, ms, tbf->direction, use_trx, 
_no);
@@ -950,11 +941,11 @@
rc = find_multi_slots(trx, ms, _slots, _slots);
if (rc < 0)
   

[PATCH] osmo-pcu[master]: Simplify TS alloc: move slot assignment

[Max]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3905

to look at the new patch set (#2).

Simplify TS alloc: move slot assignment

Move into separate functions:
* move timeslot reservation
* move UL timeslot assignment
* move DL timeslot assignment

Change-Id: I64cf78c5cfc78664766f9769dd5cde632dab92b0
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 75 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/05/3905/2

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index 47a892d..c5b398f 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -811,6 +811,76 @@
return 0;
 }
 
+/*! Update MS' reserved timeslots
+ *
+ *  \param[in,out] trx Pointer to TRX struct
+ *  \param[in,out] ms_ Pointer to MS object
+ *  \param[in] tbf_ Pointer to TBF struct
+ *  \param[in] res_ul_slots Newly reserved UL slots
+ *  \param[in] res_dl_slots Newly reserved DL slots
+ *  \param[in] ul_slots available UL slots (for logging only)
+ *  \param[in] dl_slots available DL slots (for logging only)
+ */
+static void update_ms_reserved_slots(gprs_rlcmac_trx *trx, GprsMs *ms, uint8_t 
res_ul_slots, uint8_t res_dl_slots,
+uint8_t ul_slots, uint8_t dl_slots)
+{
+   char slot_info[9] = { 0 };
+
+   if (res_ul_slots == ms->reserved_ul_slots() && res_dl_slots == 
ms->reserved_dl_slots())
+   return;
+
+   /* The reserved slots have changed, update the MS */
+   ms->set_reserved_slots(trx, res_ul_slots, res_dl_slots);
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Reserved DL/UL slots: 
(TS=0)\"%s\"(TS=7)\n",
+set_flag_chars(set_flag_chars(set_flag_chars(slot_info, dl_slots, 
'D', '.'), ul_slots, 'U'),
+   ul_slots & dl_slots, 'C'));
+}
+
+/*! Assign given UL timeslots to UL TBF
+ *
+ *  \param[in,out] ul_tbf Pointer to UL TBF struct
+ *  \param[in,out] trx Pointer to TRX object
+ *  \param[in] ul_slots Set of slots to be assigned
+ *  \param[in] tfi selected TFI
+ *  \param[in] usf selected USF
+ */
+static void assign_ul_tbf_slots(struct gprs_rlcmac_ul_tbf *ul_tbf, 
gprs_rlcmac_trx *trx, uint8_t ul_slots, int tfi,
+   int *usf)
+{
+   uint8_t ts;
+
+   for (ts = 0; ts < 8; ts++) {
+   if (!(ul_slots & (1 << ts)))
+   continue;
+
+   OSMO_ASSERT(usf[ts] >= 0);
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Assigning UL TS %u\n", ts);
+   assign_uplink_tbf_usf(>pdch[ts], ul_tbf, tfi, usf[ts]);
+   }
+}
+
+/*! Assign given DL timeslots to DL TBF
+ *
+ *  \param[in,out] dl_tbf Pointer to DL TBF struct
+ *  \param[in,out] trx Pointer to TRX object
+ *  \param[in] ul_slots Set of slots to be assigned
+ *  \param[in] tfi selected TFI
+ */
+static void assign_dl_tbf_slots(struct gprs_rlcmac_dl_tbf *dl_tbf, 
gprs_rlcmac_trx *trx, uint8_t dl_slots, int tfi)
+{
+   uint8_t ts;
+
+   for (ts = 0; ts < 8; ts++) {
+   if (!(dl_slots & (1 << ts)))
+   continue;
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Assigning DL TS %u\n", ts);
+   assign_dlink_tbf(>pdch[ts], dl_tbf, tfi);
+   }
+}
+
 /*! Slot Allocation: Algorithm B
  *
  * Assign as many downlink slots as possible.
@@ -993,51 +1063,16 @@
 * may be modified from now on. */
 
/* Step 4: Update MS and TBF and really allocate the resources */
-
-   /* The reserved slots have changed, update the MS */
-   if (reserved_ul_slots != ms->reserved_ul_slots() ||
-   reserved_dl_slots != ms->reserved_dl_slots())
-   {
-   ms_->set_reserved_slots(trx,
-   reserved_ul_slots, reserved_dl_slots);
-
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Reserved DL/UL slots: (TS=0)\"%s\"(TS=7)\n",
-   set_flag_chars(set_flag_chars(set_flag_chars(slot_info,
-   dl_slots, 'D', '.'),
-   ul_slots, 'U'),
-   ul_slots & dl_slots, 'C'));
-   }
+   update_ms_reserved_slots(trx, ms_, reserved_ul_slots, 
reserved_dl_slots, ul_slots, dl_slots);
 
tbf_->trx = trx;
tbf_->first_common_ts = first_common_ts;
tbf_->first_ts = first_ts;
 
-   if (tbf->direction == GPRS_RLCMAC_DL_TBF) {
-   struct gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf_);
-   for (ts = 0; ts < 8; ts++) {
-   if (!(dl_slots & (1 << ts)))
-   continue;
-
-   LOGP(DRLCMAC, LOGL_DEBUG, "- Assigning DL TS "
-   "%d\n", ts);
-   assign_dlink_tbf(>pdch[ts], dl_tbf, tfi);
-   }
-   } else {
-   struct gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(tbf_);
-
-   for 

[PATCH] osmo-pcu[master]: Simplify TS alloc: split allocation

[Max]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3906

to look at the new patch set (#2).

Simplify TS alloc: split allocation

* generalize TS allocation and move it into separate function
* move single-slot allocation into separate function
* use common functions for TS allocation on both UL and DL

Change-Id: Ied45ae380c345bc76fe9d6fd9a6184d1109f83f2
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 101 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/06/3906/2

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index bcac45f..70890fc 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -882,6 +882,93 @@
}
 }
 
+/*! Count used bits in slots and reserved_slots bitmasks
+ *
+ *  \param[in] slots Timeslots in use
+ *  \param[in] reserved_slots Reserved timeslots
+ *  \param[out] slotcount Number of TS in use
+ *  \param[out] avail_count Number of reserved TS
+ */
+static void update_slot_counters(uint8_t slots, uint8_t reserved_slots, 
uint8_t *slotcount, uint8_t *avail_count)
+{
+   (*slotcount) = pcu_bitcount(slots);
+   (*avail_count) = pcu_bitcount(reserved_slots);
+}
+
+/*! Return slot mask with single TS from a given UL/DL set according to TBF's 
direction, ts pointer is set to that TS
+ * number or to negative value on error
+ *
+ *  \param[in] trx Pointer to TRX object
+ *  \param[in] tbf Pointer to TBF object
+ *  \param[in] dl_slots set of DL timeslots
+ *  \param[in] ul_slots set of UL timeslots
+ *  \param[in,out] ts corresponding TS or -1 for autoselection
+ *  \returns slot mask with single UL or DL timeslot number if possible
+ */
+static uint8_t get_single_ts(const gprs_rlcmac_trx *trx, const gprs_rlcmac_tbf 
*tbf, uint8_t dl_slots, uint8_t ul_slots,
+int *ts)
+{
+   uint8_t ret = dl_slots & ul_slots; /* Make sure to consider the first 
common slot only */
+
+   if (*ts < 0)
+   *ts = find_least_busy_pdch(trx, tbf->direction, ret, 
compute_usage_by_num_tbfs, NULL, NULL);
+
+   if (*ts < 0)
+   return ffs(ret);
+
+   return ret & (1 << (*ts));
+}
+
+/*! Find set of timeslots available for allocation
+ *
+ *  \param[in] trx Pointer to TRX object
+ *  \param[in] tbf Pointer to TBF object
+ *  \param[in] single Flag to force the single TS allocation
+ *  \param[in] ul_slots set of UL timeslots
+ *  \param[in] ul_slots set of DL timeslots
+ *  \param[in] reserved_ul_slots set of reserved UL timeslots
+ *  \param[in] reserved_dl_slots set of reserved DL timeslots
+ *  \param[in] first_common_ts First TS common for both UL and DL or -1 if 
unknown
+ *  \returns negative error code or selected TS on success
+ */
+static int tbf_select_slot_set(const gprs_rlcmac_tbf *tbf, const 
gprs_rlcmac_trx *trx, bool single,
+  uint8_t ul_slots, uint8_t dl_slots,
+  uint8_t reserved_ul_slots, uint8_t 
reserved_dl_slots,
+  int8_t first_common_ts)
+{
+   uint8_t sl;
+   int ts = first_common_ts;
+   char *dir, *abrev, slot_info[9] = { 0 }, small, big;
+
+   if (tbf->direction != GPRS_RLCMAC_DL_TBF) {
+   sl = ul_slots;
+   small = 'u';
+   big = 'U';
+   dir = (char *)"uplink";
+   abrev = (char *)"UL";
+   } else {
+   sl = dl_slots;
+   small = 'd';
+   big = 'D';
+   dir = (char *)"downlink";
+   abrev = (char *)"DL";
+   }
+
+   if (single)
+   sl = get_single_ts(trx, tbf, dl_slots, ul_slots, );
+
+   if (sl == 0) {
+   LOGP(DRLCMAC, LOGL_NOTICE, "No %s slots available\n", dir);
+   return -EINVAL;
+   }
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Selected %s slots: 
(TS=0)\"%s\"(TS=7)%s\n", abrev,
+set_flag_chars(set_flag_chars(slot_info, reserved_dl_slots, small, 
'.'), sl, big),
+single ? ", single" : "");
+
+   return sl;
+}
+
 /*! Slot Allocation: Algorithm B
  *
  * Assign as many downlink slots as possible.
@@ -904,7 +991,6 @@
int8_t first_common_ts;
uint8_t slotcount = 0;
uint8_t avail_count = 0, trx_no;
-   char slot_info[9] = {0};
int ts;
int first_ts = -1;
int usf[8] = {-1, -1, -1, -1, -1, -1, -1, -1};
@@ -946,86 +1032,43 @@
reserved_dl_slots = dl_slots;
reserved_ul_slots = ul_slots;
 
-   /* Step 3: Derive the slot set for the current TBF */
-   if (single) {
-   /* Make sure to consider the first common slot only */
-   ul_slots = dl_slots = dl_slots & ul_slots;
-
-   ts = first_common_ts;
-
-   if (ts < 0)
-   ts = find_least_busy_pdch(trx, tbf->direction,
-   dl_slots & ul_slots, 

osmo-gsm-tester[master]: config: Fix combination of lists

[Neels Hofmeyr]

Patch Set 3:

(1 comment)

https://gerrit.osmocom.org/#/c/3722/3/selftest/config_test.ok
File selftest/config_test.ok:

Line 113: {'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, 
{'nominal power': '12'}], 'type': 'osmo-bts-trx'}
> what do you mean? If you only want to restrict the second one, use trx_list
ah of course, two separate trx


-- 
To view, visit https://gerrit.osmocom.org/3722
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a38f10eb9de338a77bf1fa3afceb9df1532015
Gerrit-PatchSet: 3
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

osmo-gsm-tester[master]: Use own format to specify encryption algorithm

[Neels Hofmeyr]

Patch Set 2: Code-Review+1

(1 comment)

(decide and +2 for yourself please, if you wish)

https://gerrit.osmocom.org/#/c/3815/2/selftest/template_test/osmo-nitb.cfg.tmpl
File selftest/template_test/osmo-nitb.cfg.tmpl:

Line 27
If we keep the 'a5' in here, we can leave 'encryption' as an integer and bypass 
the discussion about underscores, translation to vty format. I don't really 
have a strong opinion, you can merge this as far as I'm concerned.

(Otherwise we need some adjustment in the other encryption patch)


-- 
To view, visit https://gerrit.osmocom.org/3815
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5265cc9990dd5e99dba1f6262b3a8c597a3e958d
Gerrit-PatchSet: 2
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

osmo-gsm-tester[master]: Add cipher cfg param for modem and bts

[Neels Hofmeyr]

Patch Set 5: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3723
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0e368843a6e58bd3eeef36d2c0a7501296f0f3e
Gerrit-PatchSet: 5
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

osmo-gsm-tester[master]: Convert scenarios to restrict all objects of a given type

[Neels Hofmeyr]

Patch Set 1: Code-Review-2

> - A: scenario is just a filter for a given type of object. It
 > doesn't apply for each object in particular as in a list, but just
 > a list of attributes to require for a given type of object.

We need distinct attributes for each instance of an object. With A, e.g., I can 
only have one BTS item, so I can't select "one sysmo, one trx, please".

 > - B: Scenario is a real object-to-object specific match. That
 > means, if you have a suite.conf with 2 BTS, then you need to pass a
 > scenario which is written in a way to explicitly set attribute for
 > 2 BTS.

Yes, that is the plan. The idea is to have a scenario 
"first_bts_is_sysmo_and_second_is_trx.conf' and be able to reverse that too.

This is only possible with a list of dicts.

Could be seen as a disadvantage to need distinct configs for multi-BTS cases, 
but that was the plan from the start (it's a feature).

-- 
To view, visit https://gerrit.osmocom.org/3907
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I25eb639c7e3cf3b4c67a205422808bffbdd791e6
Gerrit-PatchSet: 1
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

osmo-gsm-tester[master]: Convert scenarios to restrict all objects of a given type

[Pau Espin Pedrol]

Patch Set 1:

For reference, discussion can be found in https://gerrit.osmocom.org/#/c/3722

After a new quick discussion, it was decided that we want to go for case B. It 
probably means this commits needs to be almost reverted, and in exchange the 
commit must do:
- Expand objects with "times" attribute > 1. Then match objects one to one, 
simply by calling combine().
- Rewrite unit tests to check different cases.
- Rewrite/rebase commits depending on this one.

-- 
To view, visit https://gerrit.osmocom.org/3907
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I25eb639c7e3cf3b4c67a205422808bffbdd791e6
Gerrit-PatchSet: 1
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

osmo-gsm-tester[master]: Convert scenarios to restrict all objects of a given type

[Neels Hofmeyr]

Patch Set 1: Code-Review-1

at first sight I don't recall us coming to this conclusion and disagree, need 
to discuss again...

-- 
To view, visit https://gerrit.osmocom.org/3907
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I25eb639c7e3cf3b4c67a205422808bffbdd791e6
Gerrit-PatchSet: 1
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-gsm-tester[master]: config: Fix combination of lists

[Pau Espin Pedrol]

Patch Set 3:

(2 comments)

https://gerrit.osmocom.org/#/c/3722/3/selftest/config_test.ok
File selftest/config_test.ok:

Line 113: {'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, 
{'nominal power': '12'}], 'type': 'osmo-bts-trx'}
> (just wondering, this requirement of two distinct nominal power values is n
what do you mean? If you only want to restrict the second one, use trx_list: 
[{}, {'nominal power': 'X'}] in scenario.


https://gerrit.osmocom.org/#/c/3722/3/src/osmo_gsm_tester/config.py
File src/osmo_gsm_tester/config.py:

Line 252: if t == dict or t == list or t == tuple:
> cosmetic hint: if t in (dict, list, tuple)
I'll apply it.


-- 
To view, visit https://gerrit.osmocom.org/3722
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a38f10eb9de338a77bf1fa3afceb9df1532015
Gerrit-PatchSet: 3
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

osmo-gsm-tester[master]: Add features attribute to modems

[Neels Hofmeyr]

Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3763
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1634049f01859ae0310174892a96e204bb670bc1
Gerrit-PatchSet: 2
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

osmo-gsm-tester[master]: config: Fix combination of lists

[Neels Hofmeyr]

Patch Set 3: Code-Review+2

(2 comments)

https://gerrit.osmocom.org/#/c/3722/3/selftest/config_test.ok
File selftest/config_test.ok:

Line 113: {'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, 
{'nominal power': '12'}], 'type': 'osmo-bts-trx'}
(just wondering, this requirement of two distinct nominal power values is not 
solvable, right?)


https://gerrit.osmocom.org/#/c/3722/3/src/osmo_gsm_tester/config.py
File src/osmo_gsm_tester/config.py:

Line 252: if t == dict or t == list or t == tuple:
cosmetic hint: if t in (dict, list, tuple)


-- 
To view, visit https://gerrit.osmocom.org/3722
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a38f10eb9de338a77bf1fa3afceb9df1532015
Gerrit-PatchSet: 3
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

osmo-gsm-tester[master]: modem: Fix race condition when connect() is called more than...

[Pau Espin Pedrol]

Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/3901/1/suites/aoip_encryption/register_a5_0_authreq.py
File suites/aoip_encryption/register_a5_0_authreq.py:

Line 34: sleep(40) # TODO: read pcap or CTRL interface and look for Rejected? 
(gsm_a.dtap.msg_mm_type == 0x04)
> in general, if at all possible, we should avoid arbitrary sleeps, we can ne
At some point I'd like having some common interface by which you can request a 
pcap file from the object, and then be able to do some checks on it through a 
class, which would call tshark or some python pcap module. In this case, we 
could check for instance that there were not Location Accept, and that there 
was at least one Location reject.

I've worked with testing frameworks which did tests then checked everything on 
recorded pcap files and it's really powerful and useful.

We could then use this kind of greps over pcap files together with test.wait() 
to poll until some event is done.


-- 
To view, visit https://gerrit.osmocom.org/3901
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4bb1c6b72c23cd860c33bee7851bca3d0ac0e1b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Neels Hofmeyr]

Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.osmocom.org/#/c/3900/1/openbsc/src/libmsc/smpp_openbsc.c
File openbsc/src/libmsc/smpp_openbsc.c:

Line 718:   sms->smpp.esme = esme;
check that we're not overwriting sms->smpp.esme if it is already set? (i.e to 
decrease the other esme->use count)


-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: neels 
Gerrit-HasComments: Yes

[MERGED] osmo-msc[master]: a_iface: fix typo

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: a_iface: fix typo
..


a_iface: fix typo

Change-Id: Ia849a4043d0fb209fe6e6840908f4f7fe90dc9e5
---
M src/libmsc/a_iface.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Max: Looks good to me, but someone else must approve
  Neels Hofmeyr: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index 8db6173..3e86494 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -153,7 +153,7 @@
LOGP(DMSC, LOGL_ERROR, "Unable to generate BSSMAP DTAP 
message!\n");
return -EINVAL;
} else
-   LOGP(DMSC, LOGL_DEBUG, "Massage will be sent as BSSMAP DTAP 
message!\n");
+   LOGP(DMSC, LOGL_DEBUG, "Message will be sent as BSSMAP DTAP 
message!\n");
 
LOGP(DMSC, LOGL_DEBUG, "N-DATA.req(%u, %s)\n", conn->a.conn_id, 
osmo_hexdump(msg_resp->data, msg_resp->len));
/* osmo_sccp_tx_data_msg() takes ownership of msg_resp */

-- 
To view, visit https://gerrit.osmocom.org/3903
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia849a4043d0fb209fe6e6840908f4f7fe90dc9e5
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 

[MERGED] osmo-msc[master]: a_iface: fix memory leaks

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: a_iface: fix memory leaks
..


a_iface: fix memory leaks

Fix multiple memory leaske in A/BSSMAP code

Change-Id: I90703c96e6a266a1cfa60b184139375aeb9ae32d
---
M include/osmocom/msc/a_iface.h
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/msc_ifaces.c
4 files changed, 21 insertions(+), 10 deletions(-)

Approvals:
  Neels Hofmeyr: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/a_iface.h b/include/osmocom/msc/a_iface.h
index a49ede2..32003eb 100644
--- a/include/osmocom/msc/a_iface.h
+++ b/include/osmocom/msc/a_iface.h
@@ -51,7 +51,7 @@
 /* Initalize A interface connection between to MSC and BSC */
 int a_init(struct osmo_sccp_instance *sccp, struct gsm_network *network);
 
-/* Send DTAP message via A-interface */
+/* Send DTAP message via A-interface, take ownership of msg */
 int a_iface_tx_dtap(struct msgb *msg);
 
 /* Send Cipher mode command via A-interface */
diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index 3d2013e..8db6173 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -127,7 +127,7 @@
return NULL;
 }
 
-/* Send DTAP message via A-interface */
+/* Send DTAP message via A-interface, take ownership of msg */
 int a_iface_tx_dtap(struct msgb *msg)
 {
struct gsm_subscriber_connection *conn;
@@ -144,6 +144,11 @@
 
msg->l3h = msg->data;
msg_resp = gsm0808_create_dtap(msg, link_id);
+
+   /* gsm0808_create_dtap() has copied the data to msg_resp,
+* so msg has served its purpose now */
+   msgb_free(msg);
+
if (!msg_resp) {
LOGP(DMSC, LOGL_ERROR, "Unable to generate BSSMAP DTAP 
message!\n");
return -EINVAL;
@@ -151,6 +156,7 @@
LOGP(DMSC, LOGL_DEBUG, "Massage will be sent as BSSMAP DTAP 
message!\n");
 
LOGP(DMSC, LOGL_DEBUG, "N-DATA.req(%u, %s)\n", conn->a.conn_id, 
osmo_hexdump(msg_resp->data, msg_resp->len));
+   /* osmo_sccp_tx_data_msg() takes ownership of msg_resp */
return osmo_sccp_tx_data_msg(conn->a.scu, conn->a.conn_id, msg_resp);
 }
 
diff --git a/src/libmsc/a_iface_bssap.c b/src/libmsc/a_iface_bssap.c
index 34d03e3..3d5755a 100644
--- a/src/libmsc/a_iface_bssap.c
+++ b/src/libmsc/a_iface_bssap.c
@@ -150,6 +150,7 @@
 
if (msgb_l3len(msg) < 1) {
LOGP(DMSC, LOGL_NOTICE, "Error: No data received -- discarding 
message!\n");
+   msgb_free(msg);
return;
}
 
@@ -327,9 +328,9 @@
conn = subscr_conn_allocate_a(a_conn_info, network, lac, scu, 
a_conn_info->conn_id);
 
/* Handover location update to the MSC code */
-   /* msc_compl_l3() takes ownership of dtap_msg
-* message buffer */
rc = msc_compl_l3(conn, msg, 0);
+   msgb_free(msg);
+
if (rc == MSC_CONN_ACCEPT) {
LOGP(DMSC, LOGL_NOTICE, "User has been accepted by MSC.\n");
return 0;
@@ -423,9 +424,9 @@
msg = NULL;
}
 
-   /* Hand over cipher mode complete message to the MSC,
-* msc_cipher_mode_compl() takes ownership for msg */
+   /* Hand over cipher mode complete message to the MSC */
msc_cipher_mode_compl(conn, msg, alg_id);
+   msgb_free(msg);
 
return 0;
 fail:
@@ -677,9 +678,9 @@
/* msc_dtap expects the dtap payload in l3h */
msg->l3h = msg->l2h + 3;
 
-   /* Forward dtap payload into the msc,
-* msc_dtap() takes ownership for msg */
+   /* Forward dtap payload into the msc */
msc_dtap(conn, conn->a.conn_id, msg);
+   msgb_free(msg);
 
return 0;
 }
@@ -696,6 +697,7 @@
if (msgb_l2len(msg) < sizeof(struct bssmap_header)) {
LOGP(DMSC, LOGL_NOTICE, "The header is too short -- discarding 
message!\n");
msgb_free(msg);
+   return -EINVAL;
}
 
switch (msg->l2h[0]) {
diff --git a/src/libmsc/msc_ifaces.c b/src/libmsc/msc_ifaces.c
index 7a04153..f54426e 100644
--- a/src/libmsc/msc_ifaces.c
+++ b/src/libmsc/msc_ifaces.c
@@ -44,10 +44,12 @@
 
 static int msc_tx(struct gsm_subscriber_connection *conn, struct msgb *msg)
 {
-   if (!conn)
-   return -EINVAL;
if (!msg)
return -EINVAL;
+   if (!conn) {
+   msgb_free(msg);
+   return -EINVAL;
+   }
 
DEBUGP(DMSC, "msc_tx %u bytes to %s via %s\n",
   msg->len, vlr_subscr_name(conn->vsub),
@@ -65,6 +67,7 @@
LOGP(DMSC, LOGL_ERROR,
 "msc_tx(): conn->via_ran invalid (%d)\n",
 conn->via_ran);
+   msgb_free(msg);
return -1;
}
 }

-- 
To view, visit https://gerrit.osmocom.org/3889
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged

osmo-gsm-tester[master]: modem: Fix race condition when connect() is called more than...

[Neels Hofmeyr]

Patch Set 2: Code-Review-1

(3 comments)

(sorry, comments on old patch set, but still apply)

-1 for the NoneType / missing None, otherwise ok. The sleep can be replaced as 
a separate issue, some time.

https://gerrit.osmocom.org/#/c/3901/1/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:

Line 357: def properties(self, *args, **kwargs):
IMHO this entire if.. including 'self.cancellable = None' should move into the 
new function


Line 524: self.cancel_pending_dbus_methods()
because now we're potentially causing a NoneType exception or forget to reset 
self.cancellable


https://gerrit.osmocom.org/#/c/3901/1/suites/aoip_encryption/register_a5_0_authreq.py
File suites/aoip_encryption/register_a5_0_authreq.py:

Line 34: sleep(40) # TODO: read pcap or CTRL interface and look for Rejected? 
(gsm_a.dtap.msg_mm_type == 0x04)
in general, if at all possible, we should avoid arbitrary sleeps, we can never 
know how long a modem will cycle around to scan... since we expect a LU reject 
here it would indeed be nice to detect a rejection. But is this possible with 
the CTRL? (listening for TRAPs / add a "rejected" flag to VLR subscriber and 
provide a new CTRL cmd for it) ... The easiest dirty hack I can think of is 
grepping the osmo-msc stderr.


-- 
To view, visit https://gerrit.osmocom.org/3901
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4bb1c6b72c23cd860c33bee7851bca3d0ac0e1b
Gerrit-PatchSet: 2
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

osmo-msc[master]: a_iface: fix typo

[Max]

Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.osmocom.org/3903
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia849a4043d0fb209fe6e6840908f4f7fe90dc9e5
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-HasComments: No

[PATCH] osmo-bsc[master]: Wrap channel state assignment in macro

[Max]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3828

to look at the new patch set (#5).

Wrap channel state assignment in macro

Previously we've used function so debug print always pointed to the same
place which is not very useful. Wrap it with macro so proper
file:line is printed. Also, make sure that we always change state only
through this wrapper and log only when the state has changed.

Change-Id: I21789f8021290965b61a54a2b23177ccbbfe8321
---
M include/osmocom/bsc/abis_rsl.h
M src/libbsc/abis_rsl.c
M src/libbsc/chan_alloc.c
3 files changed, 10 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/28/3828/5

diff --git a/include/osmocom/bsc/abis_rsl.h b/include/osmocom/bsc/abis_rsl.h
index f983fce..c092723 100644
--- a/include/osmocom/bsc/abis_rsl.h
+++ b/include/osmocom/bsc/abis_rsl.h
@@ -34,6 +34,9 @@
 
 #define GSM48_LEN2PLEN(a)  (((a) << 2) | 1)
 
+#define rsl_lchan_set_state(lch_, st_) \
+   rsl_lchan_set_state_with_log(lch_, st_, __BASE_FILE__, __LINE__)
+
 int rsl_bcch_info(const struct gsm_bts_trx *trx, enum osmo_sysinfo_type 
si_type, const uint8_t *data, int len);
 int rsl_sacch_filling(struct gsm_bts_trx *trx, uint8_t type,
  const uint8_t *data, int len);
@@ -74,7 +77,7 @@
 int rsl_release_request(struct gsm_lchan *lchan, uint8_t link_id,
enum rsl_rel_mode release_mode);
 
-int rsl_lchan_set_state(struct gsm_lchan *lchan, int);
+int rsl_lchan_set_state_with_log(struct gsm_lchan *lchan, enum gsm_lchan_state 
state, const char *file, unsigned line);
 int rsl_lchan_mark_broken(struct gsm_lchan *lchan, const char *broken);
 
 /* to be provided by external code */
diff --git a/src/libbsc/abis_rsl.c b/src/libbsc/abis_rsl.c
index 09fb14b..9968602 100644
--- a/src/libbsc/abis_rsl.c
+++ b/src/libbsc/abis_rsl.c
@@ -1225,11 +1225,12 @@
return 0;
 }
 
-int rsl_lchan_set_state(struct gsm_lchan *lchan, int state)
+int rsl_lchan_set_state_with_log(struct gsm_lchan *lchan, enum gsm_lchan_state 
state, const char *file, unsigned line)
 {
-   DEBUGP(DRSL, "%s state %s -> %s\n",
-  gsm_lchan_name(lchan), gsm_lchans_name(lchan->state),
-  gsm_lchans_name(state));
+   if (lchan->state != state)
+   LOGPSRC(DRSL, LOGL_DEBUG, file, line, "%s state %s -> %s\n",
+   gsm_lchan_name(lchan), gsm_lchans_name(lchan->state), 
gsm_lchans_name(state));
+
lchan->state = state;
return 0;
 }
diff --git a/src/libbsc/chan_alloc.c b/src/libbsc/chan_alloc.c
index 679ad42..f0275bc 100644
--- a/src/libbsc/chan_alloc.c
+++ b/src/libbsc/chan_alloc.c
@@ -448,7 +448,7 @@
osmo_timer_del(>error_timer);
 
lchan->type = GSM_LCHAN_NONE;
-   lchan->state = LCHAN_S_NONE;
+   rsl_lchan_set_state(lchan, LCHAN_S_NONE);
 
if (lchan->abis_ip.rtp_socket) {
rtp_socket_free(lchan->abis_ip.rtp_socket);

-- 
To view, visit https://gerrit.osmocom.org/3828
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21789f8021290965b61a54a2b23177ccbbfe8321
Gerrit-PatchSet: 5
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 

osmo-pcu[master]: TS alloc: use standard function

[Max]

Patch Set 1:

I've tried to implement this comparison and got really weird results:
...
EC 111.11..: {5} 3 != 4 FAIL
ED 111.11.1: {6} 1 OK
EE 111.111.: {6} 2 OK
EF 111.: {7} 1 OK
F0 : {4} 5 != 16 FAIL
F1 ...1: {5} 1 OK
F2 ..1.: {5} 2 OK
F3 ..11: {6} 1 OK
...

This prints the number in question (0..0xFF), it's bit representation, number 
of bits set, ffs() and 'OK' or psu_lsb() value if differs.

It seems like ffs() does the right thing but psu_lsb() computes every 5th value 
wrong.

Here is the code:
uint8_t u = 0, x;
do {
fprintf(stderr, "%2X " OSMO_BIT_SPEC ": {%d} %d ", u, 
OSMO_BIT_PRINT(u), pcu_bitcount(u), ffs(u));
x = pcu_lsb(u);
if (ffs(u) != x) {
fprintf(stderr, "!= %d FAIL\n", x);
} else
fprintf(stderr, "OK\n");
u++;
} while (u);

I might be missing smth but home come lsb in a byte can be more than 8? Could 
it be that using negation on unsigned type somehow confused gcc? Or some sort 
of template optimization magic went wrong?

Anyway, I think it's yet another reason to replace it with standard function 
which behaves as expected.

-- 
To view, visit https://gerrit.osmocom.org/3896
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d14ef327b09173d56ee3bca7e3ca85897d381c7
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: No

[MERGED] osmo-bsc[master]: fix vty tests: vty no longer goes to parent node implicitly

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: fix vty tests: vty no longer goes to parent node implicitly
..


fix vty tests: vty no longer goes to parent node implicitly

Fix three instances of VTY tests working because it used to include implicit
'exit' commands to the parent node.

Since libosmocore change-id Id73cba2dd34676bad8a130e9c45e67a272f19588 = commit
d64b6aed235f6e4d84a2cb8e84b32c3179260254, we no longer do this implicit-exit in
interactive VTY shells.

*) in testPingPongConfiguration, the intention is to enter the /msc 0 node.
   Drop prior entry of the 'network' node, which looks like an oversight. So
   far the 'msc 0' caused an implicit 'exit' and thus worked, now fails.

*) Two instances following comments "# Check searching for outer node's
   commands", which look like they are intended to check for this implicit-exit
   behavior. This is obsolete, drop those parts of the tests.

Change-Id: I77931d6a09c42c443c6936000592f22a7fd06cab
---
M tests/vty_test_runner.py
1 file changed, 0 insertions(+), 22 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/tests/vty_test_runner.py b/tests/vty_test_runner.py
index 1727e53..3ecfd39 100644
--- a/tests/vty_test_runner.py
+++ b/tests/vty_test_runner.py
@@ -139,14 +139,6 @@
 self.assertTrue(self.vty.verify("exit", ['']))
 self.assertTrue(self.vty.node() is None)
 
-# Check searching for outer node's commands
-self.vty.command("configure terminal")
-self.vty.command('msc 0')
-self.vty.command("bsc")
-self.assertEquals(self.vty.node(), 'config-bsc')
-self.vty.command("msc 0")
-self.assertEquals(self.vty.node(), 'config-msc')
-
 def testUssdNotificationsMsc(self):
 self.vty.enable()
 self.vty.command("configure terminal")
@@ -249,7 +241,6 @@
 def testPingPongConfiguration(self):
 self.vty.enable()
 self.vty.verify("configure terminal", [''])
-self.vty.verify("network", [''])
 self.vty.verify("msc 0", [''])
 
 self.vty.verify("timeout-ping 12", [''])
@@ -389,19 +380,6 @@
 self.assertEquals(self.vty.node(), 'config')
 self.assertTrue(self.vty.verify('exit', ['']))
 self.assertTrue(self.vty.node() is None)
-
-# Check searching for outer node's commands
-self.vty.command('configure terminal')
-self.vty.command('mgcp')
-self.vty.command('nat')
-self.assertEquals(self.vty.node(), 'config-nat')
-self.vty.command('mgcp')
-self.assertEquals(self.vty.node(), 'config-mgcp')
-self.vty.command('nat')
-self.assertEquals(self.vty.node(), 'config-nat')
-self.vty.command('bsc 0')
-self.vty.command('mgcp')
-self.assertEquals(self.vty.node(), 'config-mgcp')
 
 def testRewriteNoRewrite(self):
 self.vty.enable()

-- 
To view, visit https://gerrit.osmocom.org/3902
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I77931d6a09c42c443c6936000592f22a7fd06cab
Gerrit-PatchSet: 1
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 

osmo-pcu[master]: TS alloc: update tests

[Max]

Patch Set 6: Code-Review-1

-- 
To view, visit https://gerrit.osmocom.org/3895
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4b78951a79ddbc0745b39d091080a4e0e247d3c5
Gerrit-PatchSet: 6
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-HasComments: No

[PATCH] osmo-pcu[master]: TS alloc: update tests

[Max]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3895

to look at the new patch set (#6).

TS alloc: update tests

* restructure code for easier reading
* rearrange tests order to facilitate further UL alloc changes
* use consistent formatting for output
* log essential allocation parameters on failure

Changes to tests are combined with logging messages update to separate
code and test changes over different commits to avoid regressions.

Change-Id: I4b78951a79ddbc0745b39d091080a4e0e247d3c5
Related: OS#2282
---
M src/bts.cpp
M src/gprs_rlcmac_ts_alloc.cpp
M tests/alloc/AllocTest.cpp
M tests/alloc/AllocTest.err
M tests/alloc/AllocTest.ok
M tests/tbf/TbfTest.err
6 files changed, 3,283 insertions(+), 3,287 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/95/3895/6


-- 
To view, visit https://gerrit.osmocom.org/3895
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b78951a79ddbc0745b39d091080a4e0e247d3c5
Gerrit-PatchSet: 6
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 

osmo-gsm-tester[master]: Add features attribute to modems

[Pau Espin Pedrol]

Patch Set 2:

New version of the patch is rebased on top of changes made in arfcn branch 
which contain fixes for lists in combine().

I also added feature sms to ec20 modem as lynxis is using it and seems to be 
working correctly for him.

-- 
To view, visit https://gerrit.osmocom.org/3763
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1634049f01859ae0310174892a96e204bb670bc1
Gerrit-PatchSet: 2
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

[MERGED] osmo-sip-connector[master]: sdp.c Send octet-align in fmtp

[Keith Whyte]

Keith Whyte has submitted this change and it was merged.

Change subject: sdp.c Send octet-align in fmtp
..


sdp.c Send octet-align in fmtp

rfc4867 8.2:

  octet-align: Permissible values are 0 and 1.  If 1, octet-aligned
   operation SHALL be used.  If 0 or if not present,
   bandwidth-efficient operation is employed.

We don't have any support for AMR BE mode, but if we don't
send this the other end expects BE mode and can't decode the stream

Change-Id: I938758ac4ec55db9223e3da6c3c277e8fa670055
---
M src/sdp.c
1 file changed, 10 insertions(+), 1 deletion(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/sdp.c b/src/sdp.c
index ccd000d..213e979 100644
--- a/src/sdp.c
+++ b/src/sdp.c
@@ -166,18 +166,27 @@
 char *sdp_create_file(struct sip_call_leg *leg, struct call_leg *other)
 {
struct in_addr net = { .s_addr = ntohl(other->ip) };
+   char *fmtp_str = NULL, *sdp;
 
leg->wanted_codec = app_media_name(other->payload_msg_type);
-   return talloc_asprintf(leg,
+
+   if (strcmp(leg->wanted_codec, "AMR") == 0)
+   fmtp_str = talloc_asprintf(leg, "a=fmtp:%d octet-align=1\r\n", 
other->payload_type);
+
+   sdp = talloc_asprintf(leg,
"v=0\r\n"
"o=Osmocom 0 0 IN IP4 %s\r\n"
"s=GSM Call\r\n"
"c=IN IP4 %s\r\n"
"t=0 0\r\n"
"m=audio %d RTP/AVP %d\r\n"
+   "%s"
"a=rtpmap:%d %s/8000\r\n",
inet_ntoa(net), inet_ntoa(net), /* never use 
diff. addr! */
other->port, other->payload_type,
+   fmtp_str ? fmtp_str : "",
other->payload_type,
leg->wanted_codec);
+   talloc_free(fmtp_str);
+   return sdp;
 }

-- 
To view, visit https://gerrit.osmocom.org/3735
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I938758ac4ec55db9223e3da6c3c277e8fa670055
Gerrit-PatchSet: 11
Gerrit-Project: osmo-sip-connector
Gerrit-Branch: master
Gerrit-Owner: Keith Whyte 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 

osmo-gsm-tester[master]: Reserve ARFCN dynamically based on BTS band support

[Pau Espin Pedrol]

Patch Set 4: Code-Review-1

As we discussed, we don't want to merge this specific commit for now. We 
already described in the related task how to properly implement this.

-- 
To view, visit https://gerrit.osmocom.org/3731
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6fb5d95bed1fa50c3deaf62a7a6df3cb276bc3c9
Gerrit-PatchSet: 4
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

[PATCH] osmo-gsm-tester[master]: Use own format to specify encryption algorithm

[Pau Espin Pedrol]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3815

to look at the new patch set (#2).

Use own format to specify encryption algorithm

... instead of using the one from from osmo vty directly.

This way we avoid having multiple word attribute value and we can skip
using quotes in the conf files.

Change-Id: I5265cc9990dd5e99dba1f6262b3a8c597a3e958d
---
M example/defaults.conf
M selftest/template_test/osmo-nitb.cfg.tmpl
M src/osmo_gsm_tester/osmo_bsc.py
M src/osmo_gsm_tester/osmo_msc.py
M src/osmo_gsm_tester/osmo_nitb.py
M src/osmo_gsm_tester/util.py
M suites/aoip_encryption/register_a5_0_authopt.py
M suites/aoip_encryption/register_a5_0_authreq.py
M suites/aoip_encryption/register_a5_1_authreq.py
9 files changed, 26 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/15/3815/2

diff --git a/example/defaults.conf b/example/defaults.conf
index 082f159..969fac6 100644
--- a/example/defaults.conf
+++ b/example/defaults.conf
@@ -5,7 +5,7 @@
 short_name: osmo-gsm-tester-nitb
 long_name: osmo-gsm-tester-nitb
 auth_policy: closed
-encryption: a5 0
+encryption: a5_0
 
 bsc:
   net:
@@ -14,7 +14,7 @@
 short_name: osmo-gsm-tester-msc
 long_name: osmo-gsm-tester-msc
 auth_policy: closed
-encryption: a5 0
+encryption: a5_0
 authentication: optional
 
 msc:
@@ -24,7 +24,7 @@
 short_name: osmo-gsm-tester-msc
 long_name: osmo-gsm-tester-msc
 auth_policy: closed
-encryption: a5 0
+encryption: a5_0
 authentication: optional
 
 bsc_bts:
diff --git a/selftest/template_test/osmo-nitb.cfg.tmpl 
b/selftest/template_test/osmo-nitb.cfg.tmpl
index 200dfdc..2559b14 100644
--- a/selftest/template_test/osmo-nitb.cfg.tmpl
+++ b/selftest/template_test/osmo-nitb.cfg.tmpl
@@ -24,7 +24,7 @@
  long name ${net_name_long}
  auth policy ${net_auth_policy}
  location updating reject cause 13
- encryption a5 ${encryption}
+ encryption ${encryption}
  neci 1
  rrlp mode none
  mm info 1
diff --git a/src/osmo_gsm_tester/osmo_bsc.py b/src/osmo_gsm_tester/osmo_bsc.py
index f9eb858..5fbeea6 100644
--- a/src/osmo_gsm_tester/osmo_bsc.py
+++ b/src/osmo_gsm_tester/osmo_bsc.py
@@ -81,7 +81,10 @@
 
 # runtime parameters:
 if self.encryption is not None:
-config.overlay(values, 
dict(bsc=dict(net=dict(encryption=self.encryption
+encryption_vty = util.encryption2osmovty(self.encryption)
+else:
+encryption_vty = 
util.encryption2osmovty(values['bsc']['net']['encryption'])
+config.overlay(values, 
dict(bsc=dict(net=dict(encryption=encryption_vty
 
 self.dbg('BSC CONFIG:\n' + pprint.pformat(values))
 
diff --git a/src/osmo_gsm_tester/osmo_msc.py b/src/osmo_gsm_tester/osmo_msc.py
index f023b29..67234e3 100644
--- a/src/osmo_gsm_tester/osmo_msc.py
+++ b/src/osmo_gsm_tester/osmo_msc.py
@@ -81,7 +81,10 @@
 
 # runtime parameters:
 if self.encryption is not None:
-config.overlay(values, 
dict(msc=dict(net=dict(encryption=self.encryption
+encryption_vty = util.encryption2osmovty(self.encryption)
+else:
+encryption_vty = 
util.encryption2osmovty(values['msc']['net']['encryption'])
+config.overlay(values, 
dict(msc=dict(net=dict(encryption=encryption_vty
 if self.authentication is not None:
 config.overlay(values, 
dict(msc=dict(net=dict(authentication=self.authentication
 
diff --git a/src/osmo_gsm_tester/osmo_nitb.py b/src/osmo_gsm_tester/osmo_nitb.py
index 8f91bbd..b0a706d 100644
--- a/src/osmo_gsm_tester/osmo_nitb.py
+++ b/src/osmo_gsm_tester/osmo_nitb.py
@@ -82,7 +82,10 @@
 
 # runtime parameters:
 if self.encryption is not None:
-config.overlay(values, 
dict(nitb=dict(net=dict(encryption=self.encryption
+encryption_vty = util.encryption2osmovty(self.encryption)
+else:
+encryption_vty = 
util.encryption2osmovty(values['nitb']['net']['encryption'])
+config.overlay(values, 
dict(nitb=dict(net=dict(encryption=encryption_vty
 
 self.config = values
 
diff --git a/src/osmo_gsm_tester/util.py b/src/osmo_gsm_tester/util.py
index f3655c0..2524f47 100644
--- a/src/osmo_gsm_tester/util.py
+++ b/src/osmo_gsm_tester/util.py
@@ -337,4 +337,8 @@
 return ()
 raise ValueError('type %r not supported!' % t)
 
+def encryption2osmovty(val):
+assert val[:3] == 'a5_'
+return 'a5 ' + val[3:]
+
 # vim: expandtab tabstop=4 shiftwidth=4
diff --git a/suites/aoip_encryption/register_a5_0_authopt.py 
b/suites/aoip_encryption/register_a5_0_authopt.py
index ff93cb8..0224ee0 100755
--- a/suites/aoip_encryption/register_a5_0_authopt.py
+++ b/suites/aoip_encryption/register_a5_0_authopt.py
@@ -11,8 +11,8 @@
 
 print('start network...')
 msc.set_authentication(False)
-msc.set_encryption('a5 0')
-bsc.set_encryption('a5 0')

[PATCH] osmo-gsm-tester[master]: Reserve ARFCN dynamically based on BTS band support

[Pau Espin Pedrol]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3731

to look at the new patch set (#4).

Reserve ARFCN dynamically based on BTS band support

Instead of statically specifying a band for a BTS to use, declare a list
of supported bands for each BTS.

At the time of BTS object creation, ask the BTS for band support and try
to dynamically reserve an ARFCN resource which is compatible with any of
the bands supported by the BTS. All this happens transparently to the
test.

Still, the test may want to use a specific band / arfcn. In this case, a
test can use suite.reserve_arfcn(band, arfcn) to reserve a specific
band/arfcn and pass that to the BTS at creation time, which will then
use that one instead of trying to find a suitable one.

It is left as future work to support BTs with multiple TRX, in which
case several arfcn must be reserved. It should not be that difficult,
mostly using "times: X" where X is the amount of trx, changing the API
to use a list of arfcns and the configure() methods of the BTS.

Related: OS#2230

Change-Id: I6fb5d95bed1fa50c3deaf62a7a6df3cb276bc3c9
---
M example/default-suites.conf
M example/defaults.conf
M example/resources.conf
A example/scenarios/band-1900.conf
M selftest/conf/resources.conf
M selftest/resource_test.ok
M selftest/suite_test.ok
M selftest/suite_test/resources.conf
M src/osmo_gsm_tester/bts_osmotrx.py
M src/osmo_gsm_tester/bts_sysmo.py
M src/osmo_gsm_tester/resource.py
M src/osmo_gsm_tester/schema.py
M src/osmo_gsm_tester/suite.py
A suites/register/register_band_1900.py
A suites/register/suite.conf
15 files changed, 166 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/31/3731/4

diff --git a/example/default-suites.conf b/example/default-suites.conf
index 1e8d47a..7fc97d3 100644
--- a/example/default-suites.conf
+++ b/example/default-suites.conf
@@ -7,3 +7,4 @@
 - smpp
 - aoip_smpp
 - aoip_encryption:cipher-a50+cipher-a51
+- register:band-1900
diff --git a/example/defaults.conf b/example/defaults.conf
index 969fac6..e9e7846 100644
--- a/example/defaults.conf
+++ b/example/defaults.conf
@@ -34,7 +34,6 @@
   osmobsc_bts_type: sysmobts
   trx_list:
   - max_power_red: 0
-arfcn: 868
 timeslot_list:
 - phys_chan_config: CCCH+SDCCH4
 - phys_chan_config: SDCCH8
diff --git a/example/resources.conf b/example/resources.conf
index 95cb8cf..3ebd57f 100644
--- a/example/resources.conf
+++ b/example/resources.conf
@@ -12,14 +12,14 @@
   type: osmo-bts-sysmo
   ipa_unit_id: 1
   addr: 10.42.42.114
-  band: GSM-1800
+  bands: ['GSM-850', 'GSM-900', 'GSM-1800', 'GSM-1900']
   ciphers: [a5_0, a5_1, a5_3]
 
 - label: Ettus B200
   type: osmo-bts-trx
   ipa_unit_id: 6
   addr: 10.42.42.50
-  band: GSM-1800
+  bands: ['GSM-1800', 'GSM-1900']
   launch_trx: true
   ciphers: [a5_0, a5_1]
 
@@ -27,7 +27,7 @@
   type: osmo-bts-trx
   ipa_unit_id: 7
   addr: 10.42.42.51
-  band: GSM-1800
+  bands: ['GSM-1800']
   trx_remote_ip: 10.42.42.112
   ciphers: [a5_0, a5_1]
 
diff --git a/example/scenarios/band-1900.conf b/example/scenarios/band-1900.conf
new file mode 100644
index 000..956c8db
--- /dev/null
+++ b/example/scenarios/band-1900.conf
@@ -0,0 +1,4 @@
+resources:
+  bts:
+  - bands:
+- GSM-1900
diff --git a/selftest/conf/resources.conf b/selftest/conf/resources.conf
index b186737..e5fe6e6 100644
--- a/selftest/conf/resources.conf
+++ b/selftest/conf/resources.conf
@@ -12,7 +12,7 @@
   type: osmo-bts-sysmo
   ipa_unit_id: 1
   addr: 10.42.42.114
-  band: GSM-1800
+  bands: ['GSM-850', 'GSM-900', 'GSM-1800', 'GSM-1900']
   ciphers:
   - 'a5_0'
   - 'a5_1'
@@ -21,7 +21,7 @@
   type: osmo-bts-trx
   ipa_unit_id: 6
   addr: 10.42.42.50
-  band: GSM-1800
+  bands: ['GSM-1800']
   launch_trx: true
   ciphers:
   - 'a5_0'
@@ -31,7 +31,7 @@
   type: osmo-bts-trx
   ipa_unit_id: 7
   addr: 10.42.42.51
-  band: GSM-1800
+  bands: ['GSM-1800']
   trx_remote_ip: 10.42.42.112
   ciphers:
   - 'a5_0'
diff --git a/selftest/resource_test.ok b/selftest/resource_test.ok
index c946d3d..207cfb0 100644
--- a/selftest/resource_test.ok
+++ b/selftest/resource_test.ok
@@ -46,16 +46,16 @@
{'_hash': 'dc9ce027a257da087f31a5bc1ee6b4abd2637369',
 'arfcn': '548',
 'band': 'GSM-1900'}],
- 'bts': [{'_hash': '377ac78d5404b826d40c84efd04b4a9fd4e62b7e',
+ 'bts': [{'_hash': '2769d8f6cfe22f15e7dbd14f7ce929db2e56bdf3',
   'addr': '10.42.42.114',
-  'band': 'GSM-1800',
+  'bands': ['GSM-850', 'GSM-900', 'GSM-1800', 'GSM-1900'],
   'ciphers': ['a5_0', 'a5_1'],
   'ipa_unit_id': '1',
   'label': 'sysmoBTS 1002',
   'type': 'osmo-bts-sysmo'},
  {'_hash': '6a9c9fbd364e1563a5b9f0826030a7888fd19575',
   'addr': '10.42.42.50',
-  'band': 'GSM-1800',
+  'bands': ['GSM-1800'],
   'ciphers': ['a5_0', 'a5_1'],
   'ipa_unit_id': '6',
   'label': 

[PATCH] osmo-gsm-tester[master]: Add cipher cfg param for modem and bts

[Pau Espin Pedrol]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3723

to look at the new patch set (#5).

Add cipher cfg param for modem and bts

This parameter is contains a list of supported encryption ciphers by the
modem or bts setting it. It is so far not directly/automatically used
inside osmo-gsm-tester code, but can be useful to create scenarios for
tests that require specific ciphering modes.
For instance, aoip_encryption suite contains tests that require a BTS and
a modem that supports a5 0 and a5 1, otherwise tests will fail.

Change-Id: Ic0e368843a6e58bd3eeef36d2c0a7501296f0f3e
---
M example/default-suites.conf
M example/resources.conf
A example/scenarios/cipher-a50.conf
A example/scenarios/cipher-a51.conf
M src/osmo_gsm_tester/resource.py
M src/osmo_gsm_tester/schema.py
6 files changed, 31 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/23/3723/5

diff --git a/example/default-suites.conf b/example/default-suites.conf
index 0198486..1e8d47a 100644
--- a/example/default-suites.conf
+++ b/example/default-suites.conf
@@ -6,4 +6,4 @@
 - aoip_sms:trx-sysmocell5000
 - smpp
 - aoip_smpp
-- aoip_encryption
+- aoip_encryption:cipher-a50+cipher-a51
diff --git a/example/resources.conf b/example/resources.conf
index f190c2f..95cb8cf 100644
--- a/example/resources.conf
+++ b/example/resources.conf
@@ -13,6 +13,7 @@
   ipa_unit_id: 1
   addr: 10.42.42.114
   band: GSM-1800
+  ciphers: [a5_0, a5_1, a5_3]
 
 - label: Ettus B200
   type: osmo-bts-trx
@@ -20,6 +21,7 @@
   addr: 10.42.42.50
   band: GSM-1800
   launch_trx: true
+  ciphers: [a5_0, a5_1]
 
 - label: sysmoCell 5000
   type: osmo-bts-trx
@@ -27,6 +29,7 @@
   addr: 10.42.42.51
   band: GSM-1800
   trx_remote_ip: 10.42.42.112
+  ciphers: [a5_0, a5_1]
 
 arfcn:
   - arfcn: 512
@@ -56,21 +59,25 @@
   imsi: '90170009031'
   ki: '80A37E6FDEA931EAC92FFA5F671EFEAD'
   auth_algo: 'xor'
+  ciphers: [a5_0, a5_1]
 
 - label: sierra_2
   path: '/sierra_2'
   imsi: '90170009029'
   ki: '00969E283349D354A8239E877F2E0866'
   auth_algo: 'xor'
+  ciphers: [a5_0, a5_1]
 
 - label: gobi_0
   path: '/gobi_0'
   imsi: '90170009030'
   ki: 'BB70807226393CDBAC8DD3439FF54252'
   auth_algo: 'xor'
+  ciphers: [a5_0, a5_1]
 
 - label: gobi_3
   path: '/gobi_3'
   imsi: '90170009032'
   ki: '2F70DCA43C45ACB97E947FDD0C7CA30A'
   auth_algo: 'xor'
+  ciphers: [a5_0, a5_1]
diff --git a/example/scenarios/cipher-a50.conf 
b/example/scenarios/cipher-a50.conf
new file mode 100644
index 000..80781ed
--- /dev/null
+++ b/example/scenarios/cipher-a50.conf
@@ -0,0 +1,7 @@
+resources:
+  bts:
+ciphers:
+- a5_0
+  modem:
+ciphers:
+- a5_0
diff --git a/example/scenarios/cipher-a51.conf 
b/example/scenarios/cipher-a51.conf
new file mode 100644
index 000..a3ff872
--- /dev/null
+++ b/example/scenarios/cipher-a51.conf
@@ -0,0 +1,7 @@
+resources:
+  bts:
+ciphers:
+- a5_1
+  modem:
+ciphers:
+- a5_1
diff --git a/src/osmo_gsm_tester/resource.py b/src/osmo_gsm_tester/resource.py
index fa876d9..886c2b9 100644
--- a/src/osmo_gsm_tester/resource.py
+++ b/src/osmo_gsm_tester/resource.py
@@ -56,6 +56,7 @@
 'bts[].band': schema.BAND,
 'bts[].trx_remote_ip': schema.IPV4,
 'bts[].launch_trx': schema.BOOL_STR,
+'bts[].ciphers[]': schema.CIPHER,
 'bts[].trx_list[].hw_addr': schema.HWADDR,
 'bts[].trx_list[].net_device': schema.STR,
 'bts[].trx_list[].nominal_power': schema.INT,
@@ -66,6 +67,7 @@
 'modem[].imsi': schema.IMSI,
 'modem[].ki': schema.KI,
 'modem[].auth_algo': schema.AUTH_ALGO,
+'modem[].ciphers[]': schema.CIPHER,
 }
 
 WANT_SCHEMA = util.dict_add(
diff --git a/src/osmo_gsm_tester/schema.py b/src/osmo_gsm_tester/schema.py
index 2da80cd..cdf0a04 100644
--- a/src/osmo_gsm_tester/schema.py
+++ b/src/osmo_gsm_tester/schema.py
@@ -66,6 +66,11 @@
 return
 raise ValueError('Unknown Authentication Algorithm: %r' % val)
 
+def cipher(val):
+if val in ('a5_0', 'a5_1', 'a5_2', 'a5_3', 'a5_4', 'a5_5', 'a5_6', 'a5_7'):
+return
+raise ValueError('Unknown Cipher value: %r' % val)
+
 INT = 'int'
 STR = 'str'
 BOOL_STR = 'bool_str'
@@ -76,6 +81,7 @@
 KI = 'ki'
 MSISDN = 'msisdn'
 AUTH_ALGO = 'auth_algo'
+CIPHER = 'cipher'
 SCHEMA_TYPES = {
 INT: int,
 STR: str,
@@ -87,6 +93,7 @@
 KI: ki,
 MSISDN: msisdn,
 AUTH_ALGO: auth_algo,
+CIPHER: cipher,
 }
 
 def validate(config, schema):

-- 
To view, visit https://gerrit.osmocom.org/3723
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic0e368843a6e58bd3eeef36d2c0a7501296f0f3e
Gerrit-PatchSet: 5
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 

[PATCH] osmo-gsm-tester[master]: Convert scenarios to restrict all objects of a given type

[Pau Espin Pedrol]

Review at  https://gerrit.osmocom.org/3907

Convert scenarios to restrict all objects of a given type

Before this commit, scenarios follow the same structure as suite.conf
files:
resources: { bts: [{attrX: valX}, {attrY: valY}] }

This was meant to match objects 1-to-1 with the ones appearing in the
suite.conf. However, after discussions it was decided that what we
actually prefer for scenarios (being more convinient) is to define
a restriction we want to apply to all objects of a given type.
Otherwise, the scenario cannot be generic and needs to have knowledge on
the suite.conf details (such as amount of BTS used), and
apply restrictions for each of them.

Thus, structure of scenarios config file is change to the following:
resource: { bts: {attrX: valX} }
Then, in suite.py:combined(), we apply the dictionary "bts" for each
object of type "bts" required in suite.conf.

A unit test is added to confirm it works as expected.

During creation of this patch, it was observed that suite.conf uses a
different schema (WANT_SCHEMA), which restricts attributes which can be
specified in suite.conf. It may be useful to extend that in the future
to let suite.conf contain more attributes (as in resources.conf or
scenarios), because it may be desirable to specify already in the
suite.conf some restriction which will always need to be enforced for a
given suite to work properly. This is left for future work as for now
this can be workarouned by applying a scenario.

Change-Id: I25eb639c7e3cf3b4c67a205422808bffbdd791e6
---
M example/scenarios/sysmo.conf
M example/scenarios/trx-b200.conf
M example/scenarios/trx-sysmocell5000.conf
M example/scenarios/trx.conf
M selftest/suite_test.ok
M selftest/suite_test.py
M selftest/suite_test/resources.conf
M selftest/suite_test/test_suite/suite.conf
M src/osmo_gsm_tester/resource.py
M src/osmo_gsm_tester/suite.py
10 files changed, 146 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/07/3907/1

diff --git a/example/scenarios/sysmo.conf b/example/scenarios/sysmo.conf
index 624758b..2fff726 100644
--- a/example/scenarios/sysmo.conf
+++ b/example/scenarios/sysmo.conf
@@ -1,3 +1,3 @@
 resources:
   bts:
-  - type: osmo-bts-sysmo
+type: osmo-bts-sysmo
diff --git a/example/scenarios/trx-b200.conf b/example/scenarios/trx-b200.conf
index 2bad7e8..f57eada 100644
--- a/example/scenarios/trx-b200.conf
+++ b/example/scenarios/trx-b200.conf
@@ -1,4 +1,4 @@
 resources:
   bts:
-  - label: Ettus B200
+label: Ettus B200
 type: osmo-bts-trx
diff --git a/example/scenarios/trx-sysmocell5000.conf 
b/example/scenarios/trx-sysmocell5000.conf
index 62e9a3c..30e4ed7 100644
--- a/example/scenarios/trx-sysmocell5000.conf
+++ b/example/scenarios/trx-sysmocell5000.conf
@@ -1,4 +1,4 @@
 resources:
   bts:
-  - label: sysmoCell 5000
+label: sysmoCell 5000
 type: osmo-bts-trx
diff --git a/example/scenarios/trx.conf b/example/scenarios/trx.conf
index f1d6d13..89d0d48 100644
--- a/example/scenarios/trx.conf
+++ b/example/scenarios/trx.conf
@@ -1,3 +1,3 @@
 resources:
   bts:
-  - type: osmo-bts-trx
+type: osmo-bts-trx
diff --git a/selftest/suite_test.ok b/selftest/suite_test.ok
index 2808474..bc48a3e 100644
--- a/selftest/suite_test.ok
+++ b/selftest/suite_test.ok
@@ -16,6 +16,7 @@
 resources:
   bts:
   - times: '1'
+  - times: '2'
   ip_address:
   - times: '1'
   modem:
@@ -30,14 +31,30 @@
 -
 tst test_suite: reserving resources in 
[PATH]/selftest/suite_test/test_work/state_dir ...
 tst test_suite: DBG: {combining='resources'}
-tst {combining_scenarios='resources'}: DBG: {definition_conf={bts=[{'times': 
'1'}], ip_address=[{'times': '1'}], modem=[{'times': '2'}]}}  
[test_suite↪{combining_scenarios='resources'}]
-tst test_suite: Reserving 1 x bts (candidates: 3)
+tst {combining_scenarios='resources'}: DBG: {definition_conf={bts=[{'times': 
'1'}, {'times': '2'}], ip_address=[{'times': '1'}], modem=[{'times': '2'}]}}  
[test_suite↪{combining_scenarios='resources'}]
+tst test_suite: Reserving 3 x bts (candidates: 6)
 tst test_suite: DBG: Picked - _hash: 07d9c8aaa940b674efcbbabdd69f58a6ce4e94f9
   addr: 10.42.42.114
   band: GSM-1800
   ipa_unit_id: '1'
   label: sysmoBTS 1002
   type: sysmo
+- _hash: 76c8d2f459113cd6c99ed62d1a94bbe9a291ba94
+  addr: 10.42.42.115
+  band: GSM-1800
+  ipa_unit_id: '5'
+  label: octBTS 3000
+  trx_list:
+  - hw_addr: 00:0c:90:32:b5:8a
+  type: oct
+- _hash: 0b7fabd512b36aec43d7d496abd00af4e193b0f8
+  addr: 10.42.42.190
+  band: GSM-1900
+  ipa_unit_id: '1902'
+  label: nanoBTS 1900
+  trx_list:
+  - hw_addr: 00:02:95:00:41:b3
+  type: nanobts
 tst test_suite: Reserving 1 x ip_address (candidates: 3)
 tst test_suite: DBG: Picked - _hash: cde1debf28f07f94f92c761b4b7c6bf35785ced4
   addr: 10.42.42.1
@@ -140,5 +157,84 @@
 skip: test_error.py (N.N sec)
 skip: test_fail.py (N.N sec)
 FAIL: test_fail_raise.py (N.N sec) ExpectedFail: This 

[PATCH] osmo-gsm-tester[master]: config: Fix combination of lists

[Pau Espin Pedrol]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3722

to look at the new patch set (#3).

config: Fix combination of lists

This commit fixes combination of resources containing lists.

For lists containing complex types, it has been decided to handle them
as sorted list, where position in list matters. In this case, combine is
called recursively for each element in dest and src sharing position in
the list, and assumes that if one list is shorter than the other, then
it has to be combined against empty set for that tye.
For instance this is useful when defining trx_list properties, where a
BTS can have a different amount of TRX but we may be interested in
restricting the first TRX and don't care about extra TRX.

For lists containing simple types (eg. integers or strings), we just want
to merge both lists and we only need to check if the value is already there,
ie. handle them as unsortered sets. This case won't work if we call combine
for each element of the list because for a simple case it will just end up
checking if a[i] == b[i].
This kind of operation for simple types is needed in later commits where
cipher attribute is introduced. Without this patch, having following 2
scenarios and trying them to use together "-s foosuite:cipher-a50+ciphera51"
will fail:

cipher_a50.conf:
  bts:
  - ciphers:
- 'a5 0'

cipher_a51.conf
  bts:
  - ciphers:
- 'a5 1'

ValueError: cannot combine dicts, conflicting items (values 'a5 0' and 'a5 1')

Change-Id: Ib7a38f10eb9de338a77bf1fa3afceb9df1532015
---
M selftest/config_test.ok
M selftest/config_test.py
M src/osmo_gsm_tester/config.py
M src/osmo_gsm_tester/util.py
4 files changed, 158 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/22/3722/3

diff --git a/selftest/config_test.ok b/selftest/config_test.ok
index 80b5a06..9647deb 100644
--- a/selftest/config_test.ok
+++ b/selftest/config_test.ok
@@ -93,3 +93,31 @@
 Validation: Error
 --- imsi[]: ERR: ValueError: Invalid IMSI: None
 Validation: Error
+- Combine dicts:
+{'times': '2', 'type': 'osmo-bts-trx'}
+- Combine dicts 2:
+{'times': '1', 'label': 'foo', 'type': 'osmo-bts-trx'}
+- Combine lists:
+{'a_list': ['x', 'y', 'z']}
+- Combine lists 2:
+{'a_list': ['x', 'w', 'u', 'y', 'z']}
+- Combine lists 3:
+ValueError expected
+- Combine lists 4:
+ValueError expected
+- Combine lists 5:
+ValueError expected
+- Combine lists 6:
+{'a_list': [{}, {}]}
+- Combine lists 7:
+{'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}], 'type': 'osmo-bts-trx'}
+- Combine lists 8:
+{'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}], 'type': 'osmo-bts-trx'}
+- Combine lists 9:
+{'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}], 'type': 'osmo-bts-trx'}
+- Combine lists 10:
+{'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}], 'type': 'osmo-bts-trx'}
+- Combine lists 13:
+{'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}], 'type': 'osmo-bts-trx'}
+- Combine lists 14:
+{'times': '1', 'label': 'foo', 'trx': [], 'type': 'osmo-bts-trx'}
diff --git a/selftest/config_test.py b/selftest/config_test.py
index 61ec73a..94ac1f2 100755
--- a/selftest/config_test.py
+++ b/selftest/config_test.py
@@ -112,4 +112,94 @@
 c['imsi'][2] = None
 val(c)
 
+print('- Combine dicts:')
+a = {'times': '2'}
+b = {'type': 'osmo-bts-trx'}
+config.combine(a, b)
+print(a)
+
+print('- Combine dicts 2:')
+a = {'times': '1', 'label': 'foo', 'type': 'osmo-bts-trx'}
+b = {'type': 'osmo-bts-trx'}
+config.combine(a, b)
+print(a)
+
+print('- Combine lists:')
+a = { 'a_list': ['x', 'y', 'z'] }
+b = { 'a_list': ['y'] }
+config.combine(a, b)
+print(a)
+
+print('- Combine lists 2:')
+a = { 'a_list': ['x'] }
+b = { 'a_list': ['w', 'u', 'x', 'y', 'z'] }
+config.combine(a, b)
+print(a)
+
+print('- Combine lists 3:')
+a = { 'a_list': ['x', 3] }
+b = { 'a_list': ['y', 'z'] }
+try:
+config.combine(a, b)
+except ValueError:
+print("ValueError expected")
+
+print('- Combine lists 4:')
+a = { 'a_list': [2, 3] }
+b = { 'a_list': ['y', 'z'] }
+try:
+config.combine(a, b)
+except ValueError:
+print("ValueError expected")
+
+print('- Combine lists 5:')
+a = { 'a_list': [{}, {}] }
+b = { 'a_list': ['y', 'z'] }
+try:
+config.combine(a, b)
+except ValueError:
+print("ValueError expected")
+
+print('- Combine lists 6:')
+a = { 'a_list': [{}, {}] }
+b = { 'a_list': [{}] }
+config.combine(a, b)
+print(a)
+
+print('- Combine lists 7:')
+a = { 'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}] }
+b = { 'type': 'osmo-bts-trx', 'trx': [{'nominal power': '10'}, {'nominal 
power': '12'}] }
+config.combine(a, b)
+print(a)
+
+print('- Combine lists 8:')
+a = { 'times': '1', 'label': 'foo', 'trx': [{'nominal power': '10'}] }
+b = 

[PATCH] osmo-pcu[master]: Simplify TS alloc: move slot assignment

[Max]

Review at  https://gerrit.osmocom.org/3905

Simplify TS alloc: move slot assignment

Move into separate functionS:
* move timeslot reservation
* move UL timeslot assignment
* move DL timeslot assignment

Change-Id: I64cf78c5cfc78664766f9769dd5cde632dab92b0
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 75 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/05/3905/1

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index 47a892d..c5b398f 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -811,6 +811,76 @@
return 0;
 }
 
+/*! Update MS' reserved timeslots
+ *
+ *  \param[in,out] trx Pointer to TRX struct
+ *  \param[in,out] ms_ Pointer to MS object
+ *  \param[in] tbf_ Pointer to TBF struct
+ *  \param[in] res_ul_slots Newly reserved UL slots
+ *  \param[in] res_dl_slots Newly reserved DL slots
+ *  \param[in] ul_slots available UL slots (for logging only)
+ *  \param[in] dl_slots available DL slots (for logging only)
+ */
+static void update_ms_reserved_slots(gprs_rlcmac_trx *trx, GprsMs *ms, uint8_t 
res_ul_slots, uint8_t res_dl_slots,
+uint8_t ul_slots, uint8_t dl_slots)
+{
+   char slot_info[9] = { 0 };
+
+   if (res_ul_slots == ms->reserved_ul_slots() && res_dl_slots == 
ms->reserved_dl_slots())
+   return;
+
+   /* The reserved slots have changed, update the MS */
+   ms->set_reserved_slots(trx, res_ul_slots, res_dl_slots);
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Reserved DL/UL slots: 
(TS=0)\"%s\"(TS=7)\n",
+set_flag_chars(set_flag_chars(set_flag_chars(slot_info, dl_slots, 
'D', '.'), ul_slots, 'U'),
+   ul_slots & dl_slots, 'C'));
+}
+
+/*! Assign given UL timeslots to UL TBF
+ *
+ *  \param[in,out] ul_tbf Pointer to UL TBF struct
+ *  \param[in,out] trx Pointer to TRX object
+ *  \param[in] ul_slots Set of slots to be assigned
+ *  \param[in] tfi selected TFI
+ *  \param[in] usf selected USF
+ */
+static void assign_ul_tbf_slots(struct gprs_rlcmac_ul_tbf *ul_tbf, 
gprs_rlcmac_trx *trx, uint8_t ul_slots, int tfi,
+   int *usf)
+{
+   uint8_t ts;
+
+   for (ts = 0; ts < 8; ts++) {
+   if (!(ul_slots & (1 << ts)))
+   continue;
+
+   OSMO_ASSERT(usf[ts] >= 0);
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Assigning UL TS %u\n", ts);
+   assign_uplink_tbf_usf(>pdch[ts], ul_tbf, tfi, usf[ts]);
+   }
+}
+
+/*! Assign given DL timeslots to DL TBF
+ *
+ *  \param[in,out] dl_tbf Pointer to DL TBF struct
+ *  \param[in,out] trx Pointer to TRX object
+ *  \param[in] ul_slots Set of slots to be assigned
+ *  \param[in] tfi selected TFI
+ */
+static void assign_dl_tbf_slots(struct gprs_rlcmac_dl_tbf *dl_tbf, 
gprs_rlcmac_trx *trx, uint8_t dl_slots, int tfi)
+{
+   uint8_t ts;
+
+   for (ts = 0; ts < 8; ts++) {
+   if (!(dl_slots & (1 << ts)))
+   continue;
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Assigning DL TS %u\n", ts);
+   assign_dlink_tbf(>pdch[ts], dl_tbf, tfi);
+   }
+}
+
 /*! Slot Allocation: Algorithm B
  *
  * Assign as many downlink slots as possible.
@@ -993,51 +1063,16 @@
 * may be modified from now on. */
 
/* Step 4: Update MS and TBF and really allocate the resources */
-
-   /* The reserved slots have changed, update the MS */
-   if (reserved_ul_slots != ms->reserved_ul_slots() ||
-   reserved_dl_slots != ms->reserved_dl_slots())
-   {
-   ms_->set_reserved_slots(trx,
-   reserved_ul_slots, reserved_dl_slots);
-
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Reserved DL/UL slots: (TS=0)\"%s\"(TS=7)\n",
-   set_flag_chars(set_flag_chars(set_flag_chars(slot_info,
-   dl_slots, 'D', '.'),
-   ul_slots, 'U'),
-   ul_slots & dl_slots, 'C'));
-   }
+   update_ms_reserved_slots(trx, ms_, reserved_ul_slots, 
reserved_dl_slots, ul_slots, dl_slots);
 
tbf_->trx = trx;
tbf_->first_common_ts = first_common_ts;
tbf_->first_ts = first_ts;
 
-   if (tbf->direction == GPRS_RLCMAC_DL_TBF) {
-   struct gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf_);
-   for (ts = 0; ts < 8; ts++) {
-   if (!(dl_slots & (1 << ts)))
-   continue;
-
-   LOGP(DRLCMAC, LOGL_DEBUG, "- Assigning DL TS "
-   "%d\n", ts);
-   assign_dlink_tbf(>pdch[ts], dl_tbf, tfi);
-   }
-   } else {
-   struct gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(tbf_);
-
-   for (ts = 0; ts < 8; ts++) {
-   if (!(ul_slots & (1 << ts)))
-

[PATCH] osmo-pcu[master]: Simplify TS alloc: re-arrange code

[Max]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3760

to look at the new patch set (#9).

Simplify TS alloc: re-arrange code

* consistently format log messages to make it possible to grep for test
output in source code
* move TRX check inside local tfi_find_free() wrapper

Change-Id: I02da2b8ba8c9c8815dae0e39e1fed277ca0df171
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 16 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/60/3760/9

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index c5b398f..5ae1dfb 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -248,9 +248,7 @@
if (free_tfi) {
tfi = find_free_tfi(pdch, dir);
if (tfi < 0) {
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "no TFI available\n", ts);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping 
TS %d, because no TFI available\n", ts);
continue;
}
}
@@ -258,26 +256,20 @@
if (dir == GPRS_RLCMAC_UL_TBF) {
usf = find_free_usf(pdch);
if (usf < 0) {
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "no USF available\n", ts);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping 
TS %d, because no USF available\n", ts);
continue;
}
}
if (min_ts >= 0)
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "num TBFs %d > %d\n",
-   min_ts, min_used, num_tbfs);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping TS %d, 
because num TBFs %d > %d\n",
+min_ts, min_used, num_tbfs);
min_used = num_tbfs;
min_ts = ts;
min_tfi = tfi;
min_usf = usf;
} else {
-   LOGP(DRLCMAC, LOGL_DEBUG,
-   "- Skipping TS %d, because "
-   "num TBFs %d >= %d\n",
-   ts, num_tbfs, min_used);
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Skipping TS %d, because 
num TBFs %d >= %d\n",
+ts, num_tbfs, min_used);
}
}
 
@@ -374,7 +366,7 @@
 /*! Return free TFI
  *
  *  \param[in] bts Pointer to BTS struct
- *  \param[in] trx Pointer to TRX struct
+ *  \param[in] trx Optional pointer to TRX struct
  *  \param[in] ms Pointer to MS object
  *  \param[in] dir DL or UL direction
  *  \param[in] use_trx which TRX to use or -1 if it should be selected based 
on what MS uses
@@ -386,6 +378,15 @@
 {
int tfi;
uint8_t trx_no;
+
+   if (trx) {
+   if (use_trx >= 0 && use_trx != trx->trx_no) {
+   LOGP(DRLCMAC, LOGL_ERROR, "- Requested incompatible TRX 
%d (current is %d)\n",
+use_trx, trx->trx_no);
+   return -EINVAL;
+   }
+   use_trx = trx->trx_no;
+   }
 
if (use_trx == -1 && ms->current_trx())
use_trx = ms->current_trx()->trx_no;
@@ -924,16 +925,6 @@
reserved_ul_slots = ul_slots = ms->reserved_ul_slots();
first_common_ts = ms->first_common_ts();
trx = ms->current_trx();
-
-   if (trx) {
-   if (use_trx >= 0 && use_trx != trx->trx_no) {
-   LOGP(DRLCMAC, LOGL_ERROR,
-   "- Requested incompatible TRX %d (current is 
%d)\n",
-   use_trx, trx->trx_no);
-   return -EINVAL;
-   }
-   use_trx = trx->trx_no;
-   }
 
/* Step 2a: Find usable TRX and TFI */
tfi = tfi_find_free(bts->bts, trx, ms, tbf->direction, use_trx, 
_no);

-- 
To view, visit https://gerrit.osmocom.org/3760
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I02da2b8ba8c9c8815dae0e39e1fed277ca0df171
Gerrit-PatchSet: 9
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: 

[PATCH] osmo-pcu[master]: Simplify TS alloc: adjust function signatures

[Max]

Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3807

to look at the new patch set (#5).

Simplify TS alloc: adjust function signatures

* drop unused parameters (from both functions and structs)
* document used parameters and return values
* tighten types used for parameters
* use consistent formatting
* constify function parameters where appropriate

Tests are adjusted accordingly but test results are left untouched to
avoid regressions.

Change-Id: I39d81ab64ff790b9c4c2d0312a574485cd83e755
Related: OS#2282
---
M src/bts.cpp
M src/bts.h
M src/gprs_rlcmac.h
M src/gprs_rlcmac_ts_alloc.cpp
M src/tbf.cpp
M src/tbf.h
M src/tbf_dl.cpp
M tests/alloc/AllocTest.cpp
M tests/tbf/TbfTest.cpp
9 files changed, 147 insertions(+), 146 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/07/3807/5

diff --git a/src/bts.cpp b/src/bts.cpp
index b768569..0046238 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -439,10 +439,9 @@
  * a TRX. The first TRX that contains such an TFI is returned. Negative values
  * indicate errors.
  */
-int BTS::tfi_find_free(enum gprs_rlcmac_tbf_direction dir,
-   uint8_t *_trx, int8_t use_trx)
+int BTS::tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, 
int8_t use_trx) const
 {
-   struct gprs_rlcmac_pdch *pdch;
+   const struct gprs_rlcmac_pdch *pdch;
uint32_t free_tfis;
bool has_pdch = false;
uint8_t trx_from, trx_to, trx, ts, tfi;
@@ -652,12 +651,11 @@
/* FIXME: Copy and paste with other routines.. */
 
if (is_11bit) {
-   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0,
-   ms_class, 1);
+   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0, ms_class, 
true);
} else {
/* set class to 0, since we don't know the multislot
 * class yet */
-   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0, 0, 1);
+   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0, 0, true);
}
 
if (!tbf) {
diff --git a/src/bts.h b/src/bts.h
index d65cd2f..b3a8027 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -204,11 +204,9 @@
struct gsmtap_inst *gsmtap;
uint32_t gsmtap_categ_mask;
struct gprs_rlcmac_trx trx[8];
-   int (*alloc_algorithm)(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_tbf);
-   uint32_t alloc_algorithm_curst; /* options to customize algorithm */
+   int (*alloc_algorithm)(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, 
struct gprs_rlcmac_tbf *tbf,
+  bool single, int8_t use_tbf);
+
uint8_t force_two_phase;
uint8_t alpha, gamma;
uint8_t egprs_enabled;
@@ -366,7 +364,7 @@
gprs_rlcmac_dl_tbf *dl_tbf_by_tfi(uint8_t tfi, uint8_t trx, uint8_t ts);
gprs_rlcmac_ul_tbf *ul_tbf_by_tfi(uint8_t tfi, uint8_t trx, uint8_t ts);
 
-   int tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, 
int8_t use_trx);
+   int tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, 
int8_t use_trx) const;
 
int rcv_imm_ass_cnf(const uint8_t *data, uint32_t fn);
uint8_t is_single_block(uint16_t ra, enum ph_burst_type burst_type,
diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h
index be1e686..c16a954 100644
--- a/src/gprs_rlcmac.h
+++ b/src/gprs_rlcmac.h
@@ -21,6 +21,8 @@
 #ifndef GPRS_RLCMAC_H
 #define GPRS_RLCMAC_H
 
+#include 
+
 #ifdef __cplusplus
 #include 
 #include 
@@ -98,20 +100,14 @@
 
 extern "C" {
 #endif
-int alloc_algorithm_a(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_trx);
+int alloc_algorithm_a(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, struct 
gprs_rlcmac_tbf *tbf, bool single,
+ int8_t use_trx);
 
-int alloc_algorithm_b(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_trx);
+int alloc_algorithm_b(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, struct 
gprs_rlcmac_tbf *tbf, bool single,
+ int8_t use_trx);
 
-int alloc_algorithm_dynamic(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_trx);
+int alloc_algorithm_dynamic(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, 
struct gprs_rlcmac_tbf *tbf, bool single,
+   int8_t use_trx);
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index 57197b2..47a892d 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -103,7 +103,7 @@
return 

[PATCH] osmo-pcu[master]: Simplify TS alloc: split allocation

[Max]

Review at  https://gerrit.osmocom.org/3906

Simplify TS alloc: split allocation

* generalize TS allocation and move it into separate function
* move single-slot allocation into separate function
* use common functions for TS allocation on both UL and DL

Change-Id: Ied45ae380c345bc76fe9d6fd9a6184d1109f83f2
Related: OS#2282
---
M src/gprs_rlcmac_ts_alloc.cpp
1 file changed, 102 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/06/3906/1

diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index 5ae1dfb..6d9a27c 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -882,6 +882,93 @@
}
 }
 
+/*! Count used bits in slots and reserved_slots bitmasks
+ *
+ *  \param[in] slots Timeslots in use
+ *  \param[in] reserved_slots Reserved timeslots
+ *  \param[out] slotcount Number of TS in use
+ *  \param[out] avail_count Number of reserved TS
+ */
+static void update_slot_counters(uint8_t slots, uint8_t reserved_slots, 
uint8_t *slotcount, uint8_t *avail_count)
+{
+   (*slotcount) = pcu_bitcount(slots);
+   (*avail_count) = pcu_bitcount(reserved_slots);
+}
+
+/*! Return slot mask with single TS from a given UL/DL set according to TBF's 
direction, ts pointer is set to that TS
+ * number or to negative value on error
+ *
+ *  \param[in] trx Pointer to TRX object
+ *  \param[in] tbf Pointer to TBF object
+ *  \param[in] dl_slots set of DL timeslots
+ *  \param[in] ul_slots set of UL timeslots
+ *  \param[in,out] ts corresponding TS or -1 for autoselection
+ *  \returns slot mask with single UL or DL timeslot number if possible
+ */
+static uint8_t get_single_ts(const gprs_rlcmac_trx *trx, const gprs_rlcmac_tbf 
*tbf, uint8_t dl_slots, uint8_t ul_slots,
+int *ts)
+{
+   uint8_t ret = dl_slots & ul_slots; /* Make sure to consider the first 
common slot only */
+
+   if (*ts < 0)
+   *ts = find_least_busy_pdch(trx, tbf->direction, ret, 
compute_usage_by_num_tbfs, NULL, NULL);
+
+   if (*ts < 0)
+   return ffs(ret);
+
+   return ret & (1 << (*ts));
+}
+
+/*! Find set of timeslots available for allocation
+ *
+ *  \param[in] trx Pointer to TRX object
+ *  \param[in] tbf Pointer to TBF object
+ *  \param[in] single Flag to force the single TS allocation
+ *  \param[in] ul_slots set of UL timeslots
+ *  \param[in] ul_slots set of DL timeslots
+ *  \param[in] reserved_ul_slots set of reserved UL timeslots
+ *  \param[in] reserved_dl_slots set of reserved DL timeslots
+ *  \param[in] first_common_ts First TS common for both UL and DL or -1 if 
unknown
+ *  \returns negative error code or selected TS on success
+ */
+static int tbf_select_slot_set(const gprs_rlcmac_tbf *tbf, const 
gprs_rlcmac_trx *trx, bool single,
+  uint8_t ul_slots, uint8_t dl_slots,
+  uint8_t reserved_ul_slots, uint8_t 
reserved_dl_slots,
+  int8_t first_common_ts)
+{
+   uint8_t sl;
+   int ts = first_common_ts;
+   char *dir, *abrev, slot_info[9] = { 0 }, small, big;
+
+   if (tbf->direction != GPRS_RLCMAC_DL_TBF) {
+   sl = ul_slots;
+   small = 'u';
+   big = 'U';
+   dir = (char *)"uplink";
+   abrev = (char *)"UL";
+   } else {
+   sl = dl_slots;
+   small = 'd';
+   big = 'D';
+   dir = (char *)"downlink";
+   abrev = (char *)"DL";
+   }
+
+   if (single)
+   sl = get_single_ts(trx, tbf, dl_slots, ul_slots, );
+
+   if (sl == 0) {
+   LOGP(DRLCMAC, LOGL_NOTICE, "No %s slots available\n", dir);
+   return -EINVAL;
+   }
+
+   LOGP(DRLCMAC, LOGL_DEBUG, "- Selected %s slots: 
(TS=0)\"%s\"(TS=7)%s\n", abrev,
+set_flag_chars(set_flag_chars(slot_info, reserved_dl_slots, small, 
'.'), sl, big),
+single ? ", single" : "");
+
+   return sl;
+}
+
 /*! Slot Allocation: Algorithm B
  *
  * Assign as many downlink slots as possible.
@@ -897,14 +984,12 @@
 int alloc_algorithm_b(struct gprs_rlcmac_bts *bts, GprsMs *ms_, struct 
gprs_rlcmac_tbf *tbf_, bool single,
  int8_t use_trx)
 {
-   uint8_t dl_slots;
-   uint8_t ul_slots;
+   uint8_t dl_slots, ul_slots, slots;
uint8_t reserved_dl_slots;
uint8_t reserved_ul_slots;
int8_t first_common_ts;
uint8_t slotcount = 0;
uint8_t avail_count = 0, trx_no;
-   char slot_info[9] = {0};
int ts;
int first_ts = -1;
int usf[8] = {-1, -1, -1, -1, -1, -1, -1, -1};
@@ -946,86 +1031,43 @@
reserved_ul_slots = ul_slots;
}
 
-   /* Step 3: Derive the slot set for the current TBF */
-   if (single) {
-   /* Make sure to consider the first common slot only */
-   ul_slots = dl_slots = dl_slots & ul_slots;

[PATCH] osmo-pcu[master]: TS alloc: properly count UL slots

[Max]

Review at  https://gerrit.osmocom.org/3904

TS alloc: properly count UL slots

Add cycle to mark multiple allocated UL slots similar to the way we
count DL slots in AllocTest. Until multislot UL allocation is
implemented it does not affect test output.

Change-Id: I2705405119421da3066c6c6bdd5830df4c133a36
Related: OS#2282
---
M tests/alloc/AllocTest.cpp
1 file changed, 4 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/04/3904/1

diff --git a/tests/alloc/AllocTest.cpp b/tests/alloc/AllocTest.cpp
index 271f966..13f3869 100644
--- a/tests/alloc/AllocTest.cpp
+++ b/tests/alloc/AllocTest.cpp
@@ -600,6 +600,10 @@
if (dl_tbf->pdch[i])
dl_slots |= 1 << i;
 
+   for (i = 0; ul_tbf && i < ARRAY_SIZE(ul_tbf->pdch); i += 1)
+   if (ul_tbf->pdch[i])
+   ul_slots |= 1 << i;
+
for (i = 0; trx && i < ARRAY_SIZE(trx->pdch); i += 1) {
struct gprs_rlcmac_pdch *pdch = >pdch[i];
 

-- 
To view, visit https://gerrit.osmocom.org/3904
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2705405119421da3066c6c6bdd5830df4c133a36
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max 

[PATCH] osmo-gsm-tester[master]: modem: Fix race condition when connect() is called more than...

[Pau Espin Pedrol]

modem: Fix race condition when connect() is called more than once

An issue was spotted recently in tests run in production which call
modem.connect() twice, first time to check that we are unable to connect
with a wrong KI, then that we can connect with the correct KI.

As there's no current easy way to wait for "rejected" signal, we
basically wait for some time before checking if we were unable to
connect. It seems that sometimes waiting for 30 seconds is not enough,
and then a scan() async method is still in progress when we decide to
call connect() again, which will powercycle the modem. If the scan()
method returns at that time, then we try to access interfaces of the
modem which are no longer available because the modem is powered off at
that time. This triggers an exception and test fails.

To correct way to fix this is to make sure we disable pending Scan()
dbus method before powercycling the modem and starting a new Scan()
method.

The sleep time is also increased to make sure we let enough time to
register. It seems it can take about 15 seconds to do a full scan, and
sometimes during first scan iteration the BTS is still not found,
probably because it's still starting.

Related: OS#2510

Change-Id: Ic4bb1c6b72c23cd860c33bee7851bca3d0ac0e1b
---
M src/osmo_gsm_tester/ofono_client.py
M suites/aoip_encryption/register_a5_0_authreq.py
M suites/aoip_encryption/register_a5_1_authreq.py
3 files changed, 11 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-gsm-tester refs/changes/01/3901/2

diff --git a/src/osmo_gsm_tester/ofono_client.py 
b/src/osmo_gsm_tester/ofono_client.py
index e3858f9..30e2b55 100644
--- a/src/osmo_gsm_tester/ofono_client.py
+++ b/src/osmo_gsm_tester/ofono_client.py
@@ -349,10 +349,7 @@
 def cleanup(self):
 self.dbg('cleanup')
 if self.cancellable:
-self.cancellable.cancel()
-# Cancel op is applied as a signal coming from glib mainloop, so we
-# need to run it and wait for the callbacks to handle 
cancellations.
-poll_glib()
+self.cancel_pending_dbus_methods()
 self.cancellable = None
 self.dbus.cleanup()
 self.dbus = None
@@ -500,6 +497,12 @@
 self.log('Registering with operator', matching_op_path, mcc_mnc)
 dbus_call_dismiss_error(self, 'org.ofono.Error.InProgress', 
dbus_op.Register)
 
+def cancel_pending_dbus_methods(self):
+self.cancellable.cancel()
+# Cancel op is applied as a signal coming from glib mainloop, so we
+# need to run it and wait for the callbacks to handle cancellations.
+poll_glib()
+
 def power_cycle(self):
 'Power the modem and put it online, power cycle it if it was already 
on'
 if self.is_powered():
@@ -517,6 +520,8 @@
 'Connect to MCC+MNC'
 if (mcc_mnc is not None) and (len(mcc_mnc) != 2 or None in mcc_mnc):
 raise log.Error('mcc_mnc value is invalid. It should be None or 
contain both valid mcc and mnc values:', mcc_mnc=mcc_mnc)
+# if test called connect() before and async scanning has not finished, 
we need to get rid of it:
+self.cancel_pending_dbus_methods()
 self.power_cycle()
 self.register_attempts = 0
 if self.is_connected(mcc_mnc):
diff --git a/suites/aoip_encryption/register_a5_0_authreq.py 
b/suites/aoip_encryption/register_a5_0_authreq.py
index be8f8a1..051f5e2 100755
--- a/suites/aoip_encryption/register_a5_0_authreq.py
+++ b/suites/aoip_encryption/register_a5_0_authreq.py
@@ -31,7 +31,7 @@
 print('Attempt connection with wrong KI...')
 ms.connect(msc.mcc_mnc())
 
-sleep(30) # TODO: read pcap or CTRL interface and look for Rejected? 
(gsm_a.dtap.msg_mm_type == 0x04)
+sleep(40) # TODO: read pcap or CTRL interface and look for Rejected? 
(gsm_a.dtap.msg_mm_type == 0x04)
 print('Asserting modem did not register')
 # FIXME: this can fail because ofono qmi signals registered before being 
accepted by network. See OS#2458
 # assert not ms.is_connected(msc.mcc_mnc())
diff --git a/suites/aoip_encryption/register_a5_1_authreq.py 
b/suites/aoip_encryption/register_a5_1_authreq.py
index dd41348..11ee006 100755
--- a/suites/aoip_encryption/register_a5_1_authreq.py
+++ b/suites/aoip_encryption/register_a5_1_authreq.py
@@ -31,7 +31,7 @@
 print('Attempt connection with wrong KI...')
 ms.connect(msc.mcc_mnc())
 
-sleep(30) # TODO: read pcap or CTRL interface and look for Rejected? 
(gsm_a.dtap.msg_mm_type == 0x04)
+sleep(40) # TODO: read pcap or CTRL interface and look for Rejected? 
(gsm_a.dtap.msg_mm_type == 0x04)
 print('Asserting modem did not register')
 # FIXME: this can fail because ofono qmi signals registered before being 
accepted by network. See OS#2458
 # assert not ms.is_connected(msc.mcc_mnc())

-- 
To view, visit https://gerrit.osmocom.org/3901
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: 

[PATCH] osmo-msc[master]: a_iface: fix typo

[dexter]

Review at  https://gerrit.osmocom.org/3903

a_iface: fix typo

Change-Id: Ia849a4043d0fb209fe6e6840908f4f7fe90dc9e5
---
M src/libmsc/a_iface.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/03/3903/1

diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index 8db6173..3e86494 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -153,7 +153,7 @@
LOGP(DMSC, LOGL_ERROR, "Unable to generate BSSMAP DTAP 
message!\n");
return -EINVAL;
} else
-   LOGP(DMSC, LOGL_DEBUG, "Massage will be sent as BSSMAP DTAP 
message!\n");
+   LOGP(DMSC, LOGL_DEBUG, "Message will be sent as BSSMAP DTAP 
message!\n");
 
LOGP(DMSC, LOGL_DEBUG, "N-DATA.req(%u, %s)\n", conn->a.conn_id, 
osmo_hexdump(msg_resp->data, msg_resp->len));
/* osmo_sccp_tx_data_msg() takes ownership of msg_resp */

-- 
To view, visit https://gerrit.osmocom.org/3903
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia849a4043d0fb209fe6e6840908f4f7fe90dc9e5
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 

[PATCH] osmo-msc[master]: a_iface: fix memory leaks

[dexter]

Hello Neels Hofmeyr, Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3889

to look at the new patch set (#2).

a_iface: fix memory leaks

Fix multiple memory leaske in A/BSSMAP code

Change-Id: I90703c96e6a266a1cfa60b184139375aeb9ae32d
---
M include/osmocom/msc/a_iface.h
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/msc_ifaces.c
4 files changed, 21 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/89/3889/2

diff --git a/include/osmocom/msc/a_iface.h b/include/osmocom/msc/a_iface.h
index a49ede2..32003eb 100644
--- a/include/osmocom/msc/a_iface.h
+++ b/include/osmocom/msc/a_iface.h
@@ -51,7 +51,7 @@
 /* Initalize A interface connection between to MSC and BSC */
 int a_init(struct osmo_sccp_instance *sccp, struct gsm_network *network);
 
-/* Send DTAP message via A-interface */
+/* Send DTAP message via A-interface, take ownership of msg */
 int a_iface_tx_dtap(struct msgb *msg);
 
 /* Send Cipher mode command via A-interface */
diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index 3d2013e..8db6173 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -127,7 +127,7 @@
return NULL;
 }
 
-/* Send DTAP message via A-interface */
+/* Send DTAP message via A-interface, take ownership of msg */
 int a_iface_tx_dtap(struct msgb *msg)
 {
struct gsm_subscriber_connection *conn;
@@ -144,6 +144,11 @@
 
msg->l3h = msg->data;
msg_resp = gsm0808_create_dtap(msg, link_id);
+
+   /* gsm0808_create_dtap() has copied the data to msg_resp,
+* so msg has served its purpose now */
+   msgb_free(msg);
+
if (!msg_resp) {
LOGP(DMSC, LOGL_ERROR, "Unable to generate BSSMAP DTAP 
message!\n");
return -EINVAL;
@@ -151,6 +156,7 @@
LOGP(DMSC, LOGL_DEBUG, "Massage will be sent as BSSMAP DTAP 
message!\n");
 
LOGP(DMSC, LOGL_DEBUG, "N-DATA.req(%u, %s)\n", conn->a.conn_id, 
osmo_hexdump(msg_resp->data, msg_resp->len));
+   /* osmo_sccp_tx_data_msg() takes ownership of msg_resp */
return osmo_sccp_tx_data_msg(conn->a.scu, conn->a.conn_id, msg_resp);
 }
 
diff --git a/src/libmsc/a_iface_bssap.c b/src/libmsc/a_iface_bssap.c
index 34d03e3..3d5755a 100644
--- a/src/libmsc/a_iface_bssap.c
+++ b/src/libmsc/a_iface_bssap.c
@@ -150,6 +150,7 @@
 
if (msgb_l3len(msg) < 1) {
LOGP(DMSC, LOGL_NOTICE, "Error: No data received -- discarding 
message!\n");
+   msgb_free(msg);
return;
}
 
@@ -327,9 +328,9 @@
conn = subscr_conn_allocate_a(a_conn_info, network, lac, scu, 
a_conn_info->conn_id);
 
/* Handover location update to the MSC code */
-   /* msc_compl_l3() takes ownership of dtap_msg
-* message buffer */
rc = msc_compl_l3(conn, msg, 0);
+   msgb_free(msg);
+
if (rc == MSC_CONN_ACCEPT) {
LOGP(DMSC, LOGL_NOTICE, "User has been accepted by MSC.\n");
return 0;
@@ -423,9 +424,9 @@
msg = NULL;
}
 
-   /* Hand over cipher mode complete message to the MSC,
-* msc_cipher_mode_compl() takes ownership for msg */
+   /* Hand over cipher mode complete message to the MSC */
msc_cipher_mode_compl(conn, msg, alg_id);
+   msgb_free(msg);
 
return 0;
 fail:
@@ -677,9 +678,9 @@
/* msc_dtap expects the dtap payload in l3h */
msg->l3h = msg->l2h + 3;
 
-   /* Forward dtap payload into the msc,
-* msc_dtap() takes ownership for msg */
+   /* Forward dtap payload into the msc */
msc_dtap(conn, conn->a.conn_id, msg);
+   msgb_free(msg);
 
return 0;
 }
@@ -696,6 +697,7 @@
if (msgb_l2len(msg) < sizeof(struct bssmap_header)) {
LOGP(DMSC, LOGL_NOTICE, "The header is too short -- discarding 
message!\n");
msgb_free(msg);
+   return -EINVAL;
}
 
switch (msg->l2h[0]) {
diff --git a/src/libmsc/msc_ifaces.c b/src/libmsc/msc_ifaces.c
index 7a04153..f54426e 100644
--- a/src/libmsc/msc_ifaces.c
+++ b/src/libmsc/msc_ifaces.c
@@ -44,10 +44,12 @@
 
 static int msc_tx(struct gsm_subscriber_connection *conn, struct msgb *msg)
 {
-   if (!conn)
-   return -EINVAL;
if (!msg)
return -EINVAL;
+   if (!conn) {
+   msgb_free(msg);
+   return -EINVAL;
+   }
 
DEBUGP(DMSC, "msc_tx %u bytes to %s via %s\n",
   msg->len, vlr_subscr_name(conn->vsub),
@@ -65,6 +67,7 @@
LOGP(DMSC, LOGL_ERROR,
 "msc_tx(): conn->via_ran invalid (%d)\n",
 conn->via_ran);
+   msgb_free(msg);
return -1;
}
 }

-- 
To view, visit https://gerrit.osmocom.org/3889
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset

[PATCH] osmo-pcu[master]: Simplify TS alloc: adjust function signatures

[Max]

Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/3807

to look at the new patch set (#4).

Simplify TS alloc: adjust function signatures

* drop unused parameters (from both functions and structs)
* document used parameters and return values
* tighten types used for parameters
* use consistent formatting
* constify function parameters where appropriate

Tests are adjusted accordingly but test results are left untouched to
avoid regressions.

Change-Id: I39d81ab64ff790b9c4c2d0312a574485cd83e755
Related: OS#2400
---
M src/bts.cpp
M src/bts.h
M src/gprs_rlcmac.h
M src/gprs_rlcmac_ts_alloc.cpp
M src/tbf.cpp
M src/tbf.h
M src/tbf_dl.cpp
M tests/alloc/AllocTest.cpp
M tests/tbf/TbfTest.cpp
9 files changed, 147 insertions(+), 146 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/07/3807/4

diff --git a/src/bts.cpp b/src/bts.cpp
index b768569..0046238 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -439,10 +439,9 @@
  * a TRX. The first TRX that contains such an TFI is returned. Negative values
  * indicate errors.
  */
-int BTS::tfi_find_free(enum gprs_rlcmac_tbf_direction dir,
-   uint8_t *_trx, int8_t use_trx)
+int BTS::tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, 
int8_t use_trx) const
 {
-   struct gprs_rlcmac_pdch *pdch;
+   const struct gprs_rlcmac_pdch *pdch;
uint32_t free_tfis;
bool has_pdch = false;
uint8_t trx_from, trx_to, trx, ts, tfi;
@@ -652,12 +651,11 @@
/* FIXME: Copy and paste with other routines.. */
 
if (is_11bit) {
-   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0,
-   ms_class, 1);
+   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0, ms_class, 
true);
} else {
/* set class to 0, since we don't know the multislot
 * class yet */
-   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0, 0, 1);
+   tbf = tbf_alloc_ul_tbf(_bts, NULL, -1, 0, 0, true);
}
 
if (!tbf) {
diff --git a/src/bts.h b/src/bts.h
index d65cd2f..b3a8027 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -204,11 +204,9 @@
struct gsmtap_inst *gsmtap;
uint32_t gsmtap_categ_mask;
struct gprs_rlcmac_trx trx[8];
-   int (*alloc_algorithm)(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_tbf);
-   uint32_t alloc_algorithm_curst; /* options to customize algorithm */
+   int (*alloc_algorithm)(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, 
struct gprs_rlcmac_tbf *tbf,
+  bool single, int8_t use_tbf);
+
uint8_t force_two_phase;
uint8_t alpha, gamma;
uint8_t egprs_enabled;
@@ -366,7 +364,7 @@
gprs_rlcmac_dl_tbf *dl_tbf_by_tfi(uint8_t tfi, uint8_t trx, uint8_t ts);
gprs_rlcmac_ul_tbf *ul_tbf_by_tfi(uint8_t tfi, uint8_t trx, uint8_t ts);
 
-   int tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, 
int8_t use_trx);
+   int tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, 
int8_t use_trx) const;
 
int rcv_imm_ass_cnf(const uint8_t *data, uint32_t fn);
uint8_t is_single_block(uint16_t ra, enum ph_burst_type burst_type,
diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h
index be1e686..c16a954 100644
--- a/src/gprs_rlcmac.h
+++ b/src/gprs_rlcmac.h
@@ -21,6 +21,8 @@
 #ifndef GPRS_RLCMAC_H
 #define GPRS_RLCMAC_H
 
+#include 
+
 #ifdef __cplusplus
 #include 
 #include 
@@ -98,20 +100,14 @@
 
 extern "C" {
 #endif
-int alloc_algorithm_a(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_trx);
+int alloc_algorithm_a(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, struct 
gprs_rlcmac_tbf *tbf, bool single,
+ int8_t use_trx);
 
-int alloc_algorithm_b(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_trx);
+int alloc_algorithm_b(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, struct 
gprs_rlcmac_tbf *tbf, bool single,
+ int8_t use_trx);
 
-int alloc_algorithm_dynamic(struct gprs_rlcmac_bts *bts,
-   struct GprsMs *ms,
-   struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single,
-   int use_trx);
+int alloc_algorithm_dynamic(struct gprs_rlcmac_bts *bts, struct GprsMs *ms, 
struct gprs_rlcmac_tbf *tbf, bool single,
+   int8_t use_trx);
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp
index 57197b2..47a892d 100644
--- a/src/gprs_rlcmac_ts_alloc.cpp
+++ b/src/gprs_rlcmac_ts_alloc.cpp
@@ -103,7 +103,7 @@
return 

[MERGED] osmo-bts[master]: OML: print actual type of report sent to BSC

[Max]

Max has submitted this change and it was merged.

Change subject: OML: print actual type of report sent to BSC
..


OML: print actual type of report sent to BSC

We re-use the same facility to send different data so it's better to
print actual cause value to avoid confusion.

Change-Id: I9413ecf57eaa6fc661f1a57ccdaa2f04c50ea43b
---
M src/common/oml.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Neels Hofmeyr: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/common/oml.c b/src/common/oml.c
index 20f45e5..69a2642 100644
--- a/src/common/oml.c
+++ b/src/common/oml.c
@@ -73,7 +73,7 @@
struct msgb *nmsg;
va_list ap;
 
-   LOGP(DOML, LOGL_NOTICE, "Reporting FAILURE to BSC: ");
+   LOGP(DOML, LOGL_NOTICE, "Sending %s to BSC: ", 
get_value_string(abis_mm_event_cause_names, cause_value));
va_start(ap, fmt);
osmo_vlogp(DOML, LOGL_NOTICE, __FILE__, __LINE__, 1, fmt, ap);
nmsg = abis_nm_fail_evt_vrep(NM_EVT_PROC_FAIL, NM_SEVER_CRITICAL,

-- 
To view, visit https://gerrit.osmocom.org/3829
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9413ecf57eaa6fc661f1a57ccdaa2f04c50ea43b
Gerrit-PatchSet: 2
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 

osmo-sip-connector[master]: sdp.c Send octet-align in fmtp

[Harald Welte]

Patch Set 11: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3735
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I938758ac4ec55db9223e3da6c3c277e8fa670055
Gerrit-PatchSet: 11
Gerrit-Project: osmo-sip-connector
Gerrit-Branch: master
Gerrit-Owner: Keith Whyte 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-sip-connector[master]: sdp.c Send octet-align in fmtp

[Holger Freyther]

Patch Set 11:

@Keith: Do you want to press submit to include the change?

-- 
To view, visit https://gerrit.osmocom.org/3735
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I938758ac4ec55db9223e3da6c3c277e8fa670055
Gerrit-PatchSet: 11
Gerrit-Project: osmo-sip-connector
Gerrit-Branch: master
Gerrit-Owner: Keith Whyte 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

libosmo-sccp[master]: vty: add 'asp' / 'local-ip' command

[Harald Welte]

Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3826
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f71897dfacafcf3126e51894d6ca756b02dcd7d
Gerrit-PatchSet: 2
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: No

[MERGED] osmo-msc[master]: ctrl: subscriber-list-active: list only attached subscribers

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: ctrl: subscriber-list-active: list only attached subscribers
..


ctrl: subscriber-list-active: list only attached subscribers

I would have liked to add a regression test to verify this, but currently there
is no easy way to run CTRL tests and at the same time have access to the
osmo-msc in a way that simulates an attached subscriber.

Related: OS#2285
Change-Id: I003542b208ecf3713e9e67712d84ccb4c61af14e
---
M src/libmsc/ctrl_commands.c
1 file changed, 3 insertions(+), 0 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/libmsc/ctrl_commands.c b/src/libmsc/ctrl_commands.c
index 7e445aa..4767ddd 100644
--- a/src/libmsc/ctrl_commands.c
+++ b/src/libmsc/ctrl_commands.c
@@ -68,6 +68,9 @@
cmd->reply = talloc_strdup(cmd, "");
 
llist_for_each_entry(vsub, _ctrl_net->vlr->subscribers, list) {
+   /* Do not list subscribers that aren't successfully attached. */
+   if (!vsub->lu_complete)
+   continue;
cmd->reply = talloc_asprintf_append(cmd->reply, "%s,%s\n",
vsub->imsi, vsub->msisdn);
}

-- 
To view, visit https://gerrit.osmocom.org/3898
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I003542b208ecf3713e9e67712d84ccb4c61af14e
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder

osmo-msc[master]: ctrl: subscriber-list-active: list only attached subscribers

[Harald Welte]

Patch Set 1: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3898
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I003542b208ecf3713e9e67712d84ccb4c61af14e
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: No

[MERGED] osmocom-bb[master]: mobile/main.c: clean up config file selection logic

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: mobile/main.c: clean up config file selection logic
..


mobile/main.c: clean up config file selection logic

The 903e2515 introduced the following problems:

  - The home variable is allocated dynamically by talloc,
but not being freed. There is no need for dynamical
memory allocation, as the getenv() returns a pointer
to a value in the environment or NULL.

  - In case of custom configuration file, a pointer to
a part of stack (not heap) is passed to talloc_free().
This may cause unexpected behaviour of segfault.

Let's fix both of them.

Change-Id: I79cc3b954c3018b7e780f6351c3030c3062470b5
---
M src/host/layer23/src/mobile/main.c
1 file changed, 15 insertions(+), 9 deletions(-)

Approvals:
  Max: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/host/layer23/src/mobile/main.c 
b/src/host/layer23/src/mobile/main.c
index 4a2e4ec..bc66557 100644
--- a/src/host/layer23/src/mobile/main.c
+++ b/src/host/layer23/src/mobile/main.c
@@ -38,7 +38,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -52,8 +51,7 @@
 void *l23_ctx = NULL;
 struct llist_head ms_list;
 static char *gsmtap_ip = 0;
-static const char *config_file = ".osmocom/bb/mobile.cfg";
-bool use_default_cfg = true;
+static const char *custom_cfg_file = NULL;
 struct gsmtap_inst *gsmtap_inst = NULL;
 static char *vty_ip = "127.0.0.1";
 unsigned short vty_port = 4247;
@@ -138,8 +136,7 @@
vty_ip = optarg;
break;
case 'c':
-   config_file = optarg;
-   use_default_cfg = false;
+   custom_cfg_file = optarg;
break;
case 'v':
vty_port = atoi(optarg);
@@ -208,9 +205,9 @@
 
 int main(int argc, char **argv)
 {
+   char *config_file;
int quit = 0;
int rc;
-   char const * home;
 
printf("%s\n", openbsc_copyright);
 
@@ -240,9 +237,18 @@
gsmtap_source_add_sink(gsmtap_inst);
}
 
-   if (use_default_cfg) {
-   home = talloc_strdup(l23_ctx, getenv("HOME"));
-   config_file = talloc_asprintf_append(home, "/%s", config_file);
+   if (custom_cfg_file) {
+   /* Use full path provided by user */
+   config_file = talloc_strdup(l23_ctx, custom_cfg_file);
+   } else {
+   /* Obtain the user's home directory path */
+   const char *home_dir = getenv("HOME");
+   if (!home_dir)
+   home_dir = "~";
+
+   /* Concatenate it with default config path */
+   config_file = talloc_asprintf(l23_ctx, "%s/%s",
+   home_dir, ".osmocom/bb/mobile.cfg");
}
 
/* save the config file directory name */

-- 
To view, visit https://gerrit.osmocom.org/3897
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I79cc3b954c3018b7e780f6351c3030c3062470b5
Gerrit-PatchSet: 1
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 

osmo-bts[master]: OML: print actual type of report sent to BSC

[Harald Welte]

Patch Set 1: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3829
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9413ecf57eaa6fc661f1a57ccdaa2f04c50ea43b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-bsc[master]: Wrap channel state assignment in macro

[Harald Welte]

Patch Set 4:

(1 comment)

https://gerrit.osmocom.org/#/c/3828/4/src/libbsc/abis_rsl.c
File src/libbsc/abis_rsl.c:

Line 1228: int rsl_lchan_set_state_with_log(struct gsm_lchan *lchan, uint8_t 
state, const char *file, unsigned line)
why are we changing from an 'int' to an uint8_t here? The gsm_lchan.state 
menmber is of type "enum gsm_lchan_state", so "int" is actually closer to the 
truth than uint8_t.  And if you want to make a "proper" change, then they 
argument type should be of "enum gsm_lchan_state", no?


-- 
To view, visit https://gerrit.osmocom.org/3828
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I21789f8021290965b61a54a2b23177ccbbfe8321
Gerrit-PatchSet: 4
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Max 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Holger Freyther 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

openbsc[master]: libmsc: annotate esme route in the sms object from deliver_t...

[Harald Welte]

Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.osmocom.org/3900
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8a8fd9bbb0d3b6aff7931e4bacbea99d000e484
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

openbsc[master]: libmsc: sms_route_mt_sms() may return uninitialized return v...

[Harald Welte]

Patch Set 1: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3899
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c0bcd919cc3275d491995d17c6a32bb61c6afe1
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Keith Whyte 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 
Gerrit-HasComments: No

osmo-bsc[master]: fix vty tests: vty no longer goes to parent node implicitly

[Harald Welte]

Patch Set 1: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/3902
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I77931d6a09c42c443c6936000592f22a7fd06cab
Gerrit-PatchSet: 1
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: No