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

[XL] Change in osmo-msc[master]: ASCI: Add call control for VGCS/VBS
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: laforge, dexter. jolly has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33511 ) Change subject: ASCI: Add call control for VGCS/VBS ...................................................................... Patch Set 11: (19 comments) File include/osmocom/msc/Makefile.am: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/1821d973_f100ae98 PS10, Line 28: vgcs_fsm.h \ > maybe rename this to "msc_vgcs" (not critical, but i see all the msc_ prefixes and msc_ho. […] Done File include/osmocom/msc/vgcs_fsm.h: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/77065a89_5ad25072 PS10, Line 210: void _gsm44068_bcc_gcc_trans_free(struct gsm_trans *trans); > why does this function begin with an underscore? To me this looks like a function that is not really […] This is the handler that is called if a GCC/BCC transaction is freed. I used the same name style as for CC/SMS/SS transaction types. You are right, it is confusing. I removed that dash. File src/libmsc/vgcs_fsm.c: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/9c7398f9_16be90bd PS10, Line 123: tatic __attribute__((constructor)) void vgcs_bcc_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_bcc_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_gcc_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_gcc_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_bss_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_bss_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_cell_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_cell_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_mgw_ep_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_mgw_ep_fsm) == 0); : } > is there a reason to have separate constructor symbols for this? Why not have one consturctor functi […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/feb81000_468f9f21 PS10, Line 152: sprintf(string, "%08u", callref); > yes, 8 decimal digits plus zero termination is shorter than 16 characters, but I'd say as a matter o […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/94bf49a5_99bc4520 PS10, Line 222: if (with_prio) > also here I would first build the 32bit value as a stack variable of uint32_t type and use msgb_put_ […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/01118dc6_8583d3f8 PS10, Line 232: int _ie_invalid(void) > are those (_add_cause_ie, _add_callref_ie, _msg_too_short, _ie_invalid) really intended to be expor […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/3949c6df_7636e34f PS10, Line 249: e = msgb_put(msg, 1) > you can first buld the uint8_t as a local variable and then use msgb_put_u8(msg, value), avoiding a […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/f2629e0d_52b09a0f PS10, Line 265: #if > not sure why this is commented out? because there are no users yet and it's a static function? IF th […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/3a067ee1_6842e9b2 PS10, Line 314: payload_len > as you're decrmenting this variable, I thing I'd call it remaining_len or remaining_payload_len or s […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/7edc85f0_b1b2935a PS10, Line 323: [0] + 1; > I don't have an immediate better solution, but I really think ti is very hard to read/follow and par […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/0950e767_b4cee31a PS10, Line 337: ie[0] + > here I think you're trusting ie[0] blindly, without having done much checking on it? probably osmo_m […] I forgot ti check the return code of tlv_parse. Also I added check for each variable length IE fitting in the message. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/2dc8478c_86d879ef PS10, Line 346: ie += 4; > in each of those blocks we have a magic value (length) that is used three times and which must be co […] I replaced this "4" by the size of the IE payload. A variable "ie_len" holds the length and is then used instead of repeating the integer. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/d73c1367_9bb53d5c PS10, Line 391: ie[0] = (da << 3) > again I'd put the value together in a uint8_t local variable and then msgb_put_u8() it aftrewards. […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/182eb30a_de1d39c4 PS10, Line 416: if (ie[3] & 0x10) { > same comments as other decoders above. If you mean to check that the IE fits into the msg, its done. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/91130837_00f2cbb0 PS10, Line 559: { VGCS_GCC_EV_TIMEOUT, "Timeout due to inactiviy" }, > It probably makes more sense to stringify the constant names using OSMO_VALUE_STRING(). […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/a0d25229_30d5f3c1 PS10, Line 604: osmo_fsm_inst_free(mgw->fi); > we typically use the goto err_* label construct here to not have to repeat all the various free func […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/cc354b62_31768561 PS10, Line 694: return -EINVAL; > here we return -EINVAL without talloc_free etc? This will be done within the handling of the CLEAR event. I just added a comment that states that. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/95562024_6f95e65c PS10, Line 951: #define connect_option false > maybe use capital letters, so that it is immediately clear, that this is a define constant? Done https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/bf198b63_ffc797a1 PS10, Line 1279: int gsm44068_rcv_bcc_gcc(struct msc_a *msc_a, struct gsm_trans *trans, struct msgb *msg) > this function is quite long. Even the variable list alone fills half a page on soem screen heights. […] Done -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33511 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9947403fde8212b66758104443c60aaacc8b1e7b Gerrit-Change-Number: 33511 Gerrit-PatchSet: 11 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:26:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[L] Change in osmo-msc[master]: ASCI: Add simple implementation of Group Call Register
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: pespin. jolly has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33510 ) Change subject: ASCI: Add simple implementation of Group Call Register ...................................................................... Patch Set 9: (1 comment) File src/libmsc/asci_gcr.c: https://gerrit.osmocom.org/c/osmo-msc/+/33510/comment/364024d2_2c4c1bc9 PS8, Line 165: b = callref % pow10[l]; > OSMO_ASSERT(l < ARRAY_SIZE(pow10)); Done -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33510 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: Ia74a4a865f943c5fb388cd28f9406005c92e663e Gerrit-Change-Number: 33510 Gerrit-PatchSet: 9 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:26:14 +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-bts[master]: scheduler: use size_t for data_len in _sched_compose_*_ind()
by osmith 06 Jul '23

06 Jul '23

Attention is currently required from: laforge, fixeria. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/33585 ) Change subject: scheduler: use size_t for data_len in _sched_compose_*_ind() ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/33585 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Id75c55509a017d14dfab2a7b4c67e0742125a113 Gerrit-Change-Number: 33585 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:25:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-msc[master]: ASCI: Allow usage of rtp_stream with other FSM
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: neels, laforge, pespin. jolly has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33500 ) Change subject: ASCI: Allow usage of rtp_stream with other FSM ...................................................................... Patch Set 7: (2 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-msc/+/33500/comment/5f1aa0e7_118e66c3 PS6, Line 13: Drop the unused parent_call_leg member. > This is probably another patch. Removing the parant_call_leg is part of the patch, so it does not depend on call_leg anymore. Dropping this member is just one line that is changed. Is it really worth another patch? File include/osmocom/msc/rtp_stream.h: https://gerrit.osmocom.org/c/osmo-msc/+/33500/comment/f0d50064_068229f7 PS6, Line 29: uint32_t avail_event; > what about naming all of them ev_* or event_*. […] Done -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33500 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I0991927b6d00da08dfd455980645e68281a73a9e Gerrit-Change-Number: 33500 Gerrit-PatchSet: 7 Gerrit-Owner: jolly <andreas(a)eversberg.eu> 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: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:25:48 +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-bts[master]: scheduler: unify argument names/order for _sched_compose_*_ind()
by osmith 06 Jul '23

06 Jul '23

Attention is currently required from: laforge, fixeria. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/33583 ) Change subject: scheduler: unify argument names/order for _sched_compose_*_ind() ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/33583 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I3d8c90b82f2a55b0c5c2d6b4efb8fd962508534d Gerrit-Change-Number: 33583 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:25:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in osmo-msc[master]: ASCI: rtp_stream_commit(): Also update MGW on conn mode change
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: pespin. jolly has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33499 ) Change subject: ASCI: rtp_stream_commit(): Also update MGW on conn mode change ...................................................................... Patch Set 7: (1 comment) File src/libmsc/rtp_stream.c: https://gerrit.osmocom.org/c/osmo-msc/+/33499/comment/996c6bfd_66dfb53a PS6, Line 441: rtps->mode_sent_to_mgw = false; > imho this should only be set if "rtps->crcx_conn_mode != mode", aka new mode different than old mode […] Done -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33499 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I7a5637d0a7f1df13133e522fc78ba75eeeb2873e Gerrit-Change-Number: 33499 Gerrit-PatchSet: 7 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:25:29 +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-msc[master]: ASCI: rtp_stream_commit(): Also update MGW on conn mode change
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: jolly. Hello Jenkins Builder, laforge, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-msc/+/33499 to look at the new patch set (#7). Change subject: ASCI: rtp_stream_commit(): Also update MGW on conn mode change ...................................................................... ASCI: rtp_stream_commit(): Also update MGW on conn mode change So far rtp_stream_commit() triggers an MGCP MDCX message only when codecs or the RTP address changed. Do the same for mode changes. ('sendrecv', 'recvonly', 'sendonly',...) Change-Id: I7a5637d0a7f1df13133e522fc78ba75eeeb2873e Related: OS#4854 --- M include/osmocom/msc/rtp_stream.h M src/libmsc/call_leg.c M src/libmsc/rtp_stream.c 3 files changed, 44 insertions(+), 9 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/99/33499/7 -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33499 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I7a5637d0a7f1df13133e522fc78ba75eeeb2873e Gerrit-Change-Number: 33499 Gerrit-PatchSet: 7 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-msc[master]: ASCI: Allow usage of rtp_stream with other FSM
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: jolly, neels, laforge. Hello Jenkins Builder, neels, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-msc/+/33500 to look at the new patch set (#7). Change subject: ASCI: Allow usage of rtp_stream with other FSM ...................................................................... ASCI: Allow usage of rtp_stream with other FSM Allow the caller of rtp_stream_alloc() to define what events will be dispatched to the parent FSM. This allows other state machines to use rtp_stream. It is required for using RTP stream process with VGCS FSM. Drop the unused parent_call_leg member. Change-Id: I0991927b6d00da08dfd455980645e68281a73a9e Related: OS#4854 --- M include/osmocom/msc/rtp_stream.h M src/libmsc/call_leg.c M src/libmsc/rtp_stream.c 3 files changed, 31 insertions(+), 10 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/00/33500/7 -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33500 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I0991927b6d00da08dfd455980645e68281a73a9e Gerrit-Change-Number: 33500 Gerrit-PatchSet: 7 Gerrit-Owner: jolly <andreas(a)eversberg.eu> 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: jolly <andreas(a)eversberg.eu> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newpatchset

1 0

[L] Change in osmo-msc[master]: ASCI: Add simple implementation of Group Call Register
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: jolly. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-msc/+/33510 to look at the new patch set (#9). Change subject: ASCI: Add simple implementation of Group Call Register ...................................................................... ASCI: Add simple implementation of Group Call Register This is a built-in data structure to store and handle voice group calls. The GCR will be used by VGCS/VBS call control. (Chg-Id: I9947403fde8212b66758104443c60aaacc8b1e7b) The GCR will be used by VTY code. (Chg-Id: I5bd034a62fc8b483f550d29103c2f7587198f590) Change-Id: Ia74a4a865f943c5fb388cd28f9406005c92e663e Related: OS#4854 --- M include/osmocom/msc/Makefile.am A include/osmocom/msc/asci_gcr.h M src/libmsc/Makefile.am A src/libmsc/asci_gcr.c 4 files changed, 254 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/10/33510/9 -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33510 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: Ia74a4a865f943c5fb388cd28f9406005c92e663e Gerrit-Change-Number: 33510 Gerrit-PatchSet: 9 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-MessageType: newpatchset

1 0

[XL] Change in osmo-msc[master]: ASCI: Add call control for VGCS/VBS
by jolly 06 Jul '23

06 Jul '23

Attention is currently required from: jolly. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-msc/+/33511 to look at the new patch set (#11). Change subject: ASCI: Add call control for VGCS/VBS ...................................................................... ASCI: Add call control for VGCS/VBS Change-Id: I9947403fde8212b66758104443c60aaacc8b1e7b Related: OS#4854 --- M include/osmocom/msc/Makefile.am A include/osmocom/msc/msc_vgcs.h M include/osmocom/msc/ran_conn.h M include/osmocom/msc/transaction.h M src/libmsc/Makefile.am M src/libmsc/gsm_04_11_gsup.c A src/libmsc/msc_vgcs.c M src/libmsc/transaction.c 8 files changed, 3,033 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/33511/11 -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33511 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9947403fde8212b66758104443c60aaacc8b1e7b Gerrit-Change-Number: 33511 Gerrit-PatchSet: 11 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-MessageType: newpatchset

1 0