March 2023 - gerrit-log - lists.osmocom.org

[S] Change in osmo-bts[master]: osmo-bts-trx: log TRXC/TRXD connection address
by msuraev 04 Mar '23

04 Mar '23

msuraev has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31530 ) Change subject: osmo-bts-trx: log TRXC/TRXD connection address ...................................................................... osmo-bts-trx: log TRXC/TRXD connection address Change-Id: Id53c1dd58b95c45f261fa9bfae16cc53b20edd18 --- M src/osmo-bts-trx/trx_if.c 1 file changed, 12 insertions(+), 2 deletions(-) Approvals: fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve msuraev: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/osmo-bts-trx/trx_if.c b/src/osmo-bts-trx/trx_if.c index bb9404c..3446018 100644 --- a/src/osmo-bts-trx/trx_if.c +++ b/src/osmo-bts-trx/trx_if.c @@ -1232,7 +1232,8 @@ /*! close the TRX for given handle (data + control socket) */ void trx_if_close(struct trx_l1h *l1h) { - LOGPPHI(l1h->phy_inst, DTRX, LOGL_NOTICE, "Closing TRXC/TRXD connections\n"); + LOGPPHI(l1h->phy_inst, DTRX, LOGL_NOTICE, "Closing TRXC/TRXD connections to %s\n", + l1h->phy_inst->phy_link->u.osmotrx.remote_ip); trx_if_flush(l1h); @@ -1263,7 +1264,7 @@ struct phy_link *plink = pinst->phy_link; int rc; - LOGPPHI(pinst, DTRX, LOGL_NOTICE, "Opening TRXC/TRXD connections\n"); + LOGPPHI(pinst, DTRX, LOGL_NOTICE, "Opening TRXC/TRXD connections to %s\n", plink->u.osmotrx.remote_ip); /* open sockets */ rc = trx_udp_open(l1h, &l1h->trx_ofd_ctrl, -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31530 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Id53c1dd58b95c45f261fa9bfae16cc53b20edd18 Gerrit-Change-Number: 31530 Gerrit-PatchSet: 2 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in osmo-bts[master]: osmo-bts-trx: use bool for true/false flags
by msuraev 04 Mar '23

04 Mar '23

msuraev has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31531 ) Change subject: osmo-bts-trx: use bool for true/false flags ...................................................................... osmo-bts-trx: use bool for true/false flags Change-Id: Iaa97dc36e92797db5f348e0bb79d2191ca0a58b5 --- M src/osmo-bts-trx/trx_if.c 1 file changed, 14 insertions(+), 5 deletions(-) Approvals: fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/osmo-bts-trx/trx_if.c b/src/osmo-bts-trx/trx_if.c index 3446018..59603ab 100644 --- a/src/osmo-bts-trx/trx_if.c +++ b/src/osmo-bts-trx/trx_if.c @@ -1243,7 +1243,7 @@ } /*! compute UDP port number used for TRX protocol */ -static uint16_t compute_port(struct phy_instance *pinst, int remote, int is_data) +static uint16_t compute_port(struct phy_instance *pinst, bool remote, bool is_data) { struct phy_link *plink = pinst->phy_link; uint16_t inc = 1; @@ -1269,16 +1269,16 @@ /* open sockets */ rc = trx_udp_open(l1h, &l1h->trx_ofd_ctrl, plink->u.osmotrx.local_ip, - compute_port(pinst, 0, 0), + compute_port(pinst, false, false), plink->u.osmotrx.remote_ip, - compute_port(pinst, 1, 0), trx_ctrl_read_cb); + compute_port(pinst, true, false), trx_ctrl_read_cb); if (rc < 0) return rc; rc = trx_udp_open(l1h, &l1h->trx_ofd_data, plink->u.osmotrx.local_ip, - compute_port(pinst, 0, 1), + compute_port(pinst, false, true), plink->u.osmotrx.remote_ip, - compute_port(pinst, 1, 1), trx_data_read_cb); + compute_port(pinst, true, true), trx_data_read_cb); if (rc < 0) return rc; -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31531 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Iaa97dc36e92797db5f348e0bb79d2191ca0a58b5 Gerrit-Change-Number: 31531 Gerrit-PatchSet: 2 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in osmo-bts[master]: osmo-bts-trx: log TRXC/TRXD connection address
by msuraev 04 Mar '23

04 Mar '23

msuraev has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31530 ) Change subject: osmo-bts-trx: log TRXC/TRXD connection address ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31530 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Id53c1dd58b95c45f261fa9bfae16cc53b20edd18 Gerrit-Change-Number: 31530 Gerrit-PatchSet: 1 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 11:18:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-msc[master]: SMPP: refactor ref counting code
by msuraev 04 Mar '23

04 Mar '23

Attention is currently required from: fixeria, pespin. msuraev has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/30683 ) Change subject: SMPP: refactor ref counting code ...................................................................... Patch Set 3: (1 comment) Commit Message: https://gerrit.osmocom.org/c/osmo-msc/+/30683/comment/26414553_108b803f PS1, Line 9: single-use static function to simplify > There's nothing wrong with the function. […] Done -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/30683 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9cb530991f54aa78edaa885f1242f63c705b6fcb Gerrit-Change-Number: 30683 Gerrit-PatchSet: 3 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 11:17:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Comment-In-Reply-To: msuraev <msuraev(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in osmo-bsc[master]: pcuif_proto: increment version number
by neels 04 Mar '23

04 Mar '23

Attention is currently required from: laforge, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31597 ) Change subject: pcuif_proto: increment version number ...................................................................... Patch Set 6: Code-Review+1 (2 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bsc/+/31597/comment/ac36e0c8_1a3b3db5 PS6, Line 10: lets ("let's") Patchset: PS1: > I will put this to WIP to be sure. currently there is no prevention of accidental merge in place -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31597 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I8315bd67c7f3eb0d7ee71b64cd4dff889a84fcf1 Gerrit-Change-Number: 31597 Gerrit-PatchSet: 6 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 00:54:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: handle multiple BTSs with one BSC co-located PCU (in theory)
by neels 04 Mar '23

04 Mar '23

Attention is currently required from: laforge, pespin, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31618 ) Change subject: pcu_sock: handle multiple BTSs with one BSC co-located PCU (in theory) ...................................................................... Patch Set 5: (9 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/e1b1b1a8_c80c6fcc PS5, Line 15: RBS) are configured. how about an osmo-bsc with one Ericsson RBS co-located, and then a bunch of other types of BTS with no BSC-co-located PCU? The position of the Ericsson RBS could be anywhere in the bts_list. The reasoning seems to be exclusively about Ericsson RBS. Patchset: PS5: > You are still preventing to configure more than 1 ericsson BTS here. […] pau, can you point out the place? i don't see it... File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/1b76bb41_82bdf365 PS5, Line 810: llist_for_each_entry(bts, &state->net->bts_list, list) { could there be several RBS with several separate co-located PCUs, each with their own pcu_sock? This would then disconnect all of them if only one disconnects. I'm asking because in pcu_sock_init() it seems like one pcu_sock is assigned to a single bts bts->pcu_state = state so seems to me the relation of pcu_sock to bts should be 1:1. Here it is handled as 1:N instead. https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/1262db71_ef50ee98 PS5, Line 960: (unusual whitespace after the type cast brace) https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/088c4228_d0f2d5c2 PS5, Line 960: rc (would prefer if the return val were named 'fd', because 'man accept' says RETURN VALUE On success, these system calls return a file descriptor [...] and below "close(rc)" looks weird, "close(fd)" would make more sense) https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/f1c3cc60_a4a2eba0 PS5, Line 966: if (conn_bfd->fd >= 0) { should this check happen before accept(), so we don't even accept a new connection when there already is one https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/921a7285_9b3374b2 PS5, Line 969: osmo_fd_read_disable(&state->listen_bfd); so this disables the other active connection?? https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/5a4cc95e_bb4afb34 PS5, Line 986: llist_for_each_entry(bts, &state->net->bts_list, list) { same question as above, pcu to bts relation is 1:1 or 1:N? I guess you should pass the bts pointer that the pcu socket belongs to in pcu_sock_state and only act on that specific bts here. https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/bfd9e82c_2651145f PS5, Line 1032: bts->pcu_state = state; here it is pcu to bts relation 1:1? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31618 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I0b42c2c130106f6ffca2dd08d079e1a7bda41f0b Gerrit-Change-Number: 31618 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 00:52:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: improve logging
by neels 04 Mar '23

04 Mar '23

Attention is currently required from: dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31579 ) Change subject: pcu_sock: improve logging ...................................................................... Patch Set 8: (1 comment) File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31579/comment/213a9e21_2321d521 PS8, Line 417: BTS %d drop this, already included in LOG_BTS -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31579 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I4a3ff23ffccf803b1a97e3f2604d3b422c6d6afd Gerrit-Change-Number: 31579 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 00:28:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH
by neels 04 Mar '23

04 Mar '23

Attention is currently required from: pespin, fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31578 ) Change subject: pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH ...................................................................... Patch Set 8: (4 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/af14cb7f_934b30d7 PS8, Line 10: "direct TLLI" method, the TLLI (and the paging group) is premended to > this needs updating. We are sending the IMSI now. […] probably "prepended" File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/b899a834_0a6023a9 PS8, Line 579: pch_dt->tlli, pch_dt->imsi, pag_grp); > wrong indentation you keep saying that, but single-tab indent is a perfectly fine alternative to indenting lined up with the opening brace, and I keep saying that. https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/218fd7c8_e18cce71 PS8, Line 581: msg = msgb_alloc(sizeof(pch_dt->data), "pcu_pch"); > why creating a new msg and copying the contents? Can you simply remove the header from the existing […] this is just moving code from a previous patch, but in fact this msgb is leaked! It should have been freed in all code paths, rc or !rc. (There seems to be no existing msgb to remove the header of, pch_dt is not a msgb) But since you don't pass msg as arg anywhere, but only pass its data and len, might as well just pass the pch_dt->data and its size to rsl_ericsson_imm_assign_cmd() below. That skips the unnecessary msgb allocation and hence there can be no msgb leak. And since this problem exists before this patch, rather submit one separate patch before this, to fix the msgb thing on its own, so that this patch only shows the intended functional change. https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/ab4c0b6b_e8bc311c PS8, Line 595: msg->len, msg->data ...here just pass the pch_dt->data -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31578 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id6acbd243adf26169e5e8319dd66bb68dd6a3c22 Gerrit-Change-Number: 31578 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 00:23:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in osmo-bsc[master]: cosmetic: timeslot_fsm.c: move some code to separate function
by neels 04 Mar '23

04 Mar '23

Attention is currently required from: dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31658 ) Change subject: cosmetic: timeslot_fsm.c: move some code to separate function ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31658 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I2c26d937496211e4b876987bac3803f6b2e6c8be Gerrit-Change-Number: 31658 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 00:10:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: activate/deactivate PDCH on pcu reconnect
by neels 04 Mar '23

04 Mar '23

Attention is currently required from: fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31497 ) Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect ...................................................................... Patch Set 10: (3 comments) Patchset: PS10: excellent, +2 from me, just that reference to a non existent function... File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/0506925e_cbd6f8b5 PS10, Line 971: pdch_deact_bts there is no pdch_deact_bts() ... (i'd just write the comment again / ok if you want to fix the reference instead) File src/osmo-bsc/timeslot_fsm.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/34734080_db5a332d PS9, Line 1128: NOTE > I like to use it to mark comments which I think are more important then other comments. […] it always throws me off a bit ... like, if a comment is not noteworthy, then we can drop the comment ... it's an almost certain way to know that you are the author of the code =) -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678 Gerrit-Change-Number: 31497 Gerrit-PatchSet: 10 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 04 Mar 2023 00:08:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: comment

1 0