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

[M] Change in osmo-bsc[master]: simplify storage of bsc_msc_data->audio_support
by neels 03 Mar '23

03 Mar '23

Attention is currently required from: laforge, pespin, fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31609 ) Change subject: simplify storage of bsc_msc_data->audio_support ...................................................................... Patch Set 3: (1 comment) File include/osmocom/bsc/bsc_msc_data.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31609/comment/1baaf1c6_c22609e0 PS1, Line 140: struct gsm_audio_support audio_support[16]; > MY concern regarding 13 vs 16 has not been answered afaict? i changed the comment, apparently forgot to push -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31609 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I625cedc4bb040d649fd6e1794ba468f4c6ad6adc Gerrit-Change-Number: 31609 Gerrit-PatchSet: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Fri, 03 Mar 2023 00:56:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: Includes: Remove enum defs, instead include them
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31304 ) Change subject: Includes: Remove enum defs, instead include them ...................................................................... Patch Set 4: (1 comment) This change is ready for review. File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31304/comment/522ed9ff_1adb7ab1 PS1, Line 654: enum gprs_cs { > AFAICT this is only used internally and never sent into the wire. […] This enum pretty much already exists in `libosmocore/include/osmocom/gsm/protocol/gsm_44_060.h`. What's the reason not to use it? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31304 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I2883aee12b501a717d5acecd5638882724336e72 Gerrit-Change-Number: 31304 Gerrit-PatchSet: 4 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 02 Mar 2023 23:00:17 +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-trx[master]: Transition to use of 'telnet_init_default'
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: daniel, msuraev. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/31526 ) Change subject: Transition to use of 'telnet_init_default' ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: endianness checks have been fixed by Oliver (iirc), so Jenkins doesn't fail anymore -- To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/31526 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-Change-Id: Icc57c68337d55c6594c1c36e9bf41624d11dab0a Gerrit-Change-Number: 31526 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Thu, 02 Mar 2023 22:26:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: osmo_wqueue_init(): Fix parameter type
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31595 ) Change subject: osmo_wqueue_init(): Fix parameter type ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > A simple typecast means all the potential users having to compile the app on their systems using som […] Maybe there's a misunderstanding - I wasn't suggesting we add a typecast somewhere in the osmo_wqeue API -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31595 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I6c9295750e1755bf1d80f3a3b32efc2aefb11a57 Gerrit-Change-Number: 31595 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 02 Mar 2023 22:22:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bts[master]: common: Make socket queue max. length configurable
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31534 ) Change subject: common: Make socket queue max. length configurable ...................................................................... Patch Set 5: (2 comments) File src/common/vty.c: https://gerrit.osmocom.org/c/osmo-bts/+/31534/comment/cfc50cb7_ab5b86aa PS4, Line 467: vty_out(vty, " pcu-socket-queue-length %d%s", bts->pcu.sock_qlength_max, VTY_NEWLINE); > wqueue Done https://gerrit.osmocom.org/c/osmo-bts/+/31534/comment/9e6c4347_9e626e39 PS4, Line 985: "pcu-socket-queue-length <1-" OSMO_STRINGIFY_VAL(BTS_CFG_PCU_SOCK_QLENGTH_MAX_MAX) ">", > why is 128 the maximum? Simply use a big number which can fit in sock_qlength_max, like 2^32-1 or al […] Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31534 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Id6ba6e4eadce9ce82ef2407f4e28346e7fe4abfa Gerrit-Change-Number: 31534 Gerrit-PatchSet: 5 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 02 Mar 2023 22:17:49 +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]: common: Make socket queue max. length configurable
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: arehbein. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bts/+/31534 to look at the new patch set (#6). Change subject: common: Make socket queue max. length configurable ...................................................................... common: Make socket queue max. length configurable Title refers to the maximum length of the osmo_wqueue used for the PCU socket connection. Requires: Change Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Related: OS#5774 Change-Id: Id6ba6e4eadce9ce82ef2407f4e28346e7fe4abfa --- M include/osmo-bts/bts.h M include/osmo-bts/pcu_if.h M src/common/bts.c M src/common/main.c M src/common/pcu_sock.c M src/common/vty.c M tests/osmo-bts.vty 7 files changed, 82 insertions(+), 28 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/34/31534/6 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31534 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Id6ba6e4eadce9ce82ef2407f4e28346e7fe4abfa Gerrit-Change-Number: 31534 Gerrit-PatchSet: 6 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-bts[master]: common: Have PCU socket connection use osmo_wqueue
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31533 ) Change subject: common: Have PCU socket connection use osmo_wqueue ...................................................................... Patch Set 4: (6 comments) File include/osmo-bts/pcuif_proto.h: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/f10f9ec7_96d976af PS2, Line 8: #define PCU_SOCK_QLENGTH_MAX_DEFAULT 10 > Ack Ah or course I see what you mean, thanks for the explanation. I have put it in `bts.h`. It might also make sense to move it to libosmocore, because 10 seems to be the implicit default wqueue length (I grepped for calls to `osmo_wqueue_setup()`) for Osmocom's pcu, bsc and bts. File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/3cda8dd9_45754a6b PS2, Line 1092: return -1; > osmo_wqueue_bfd_cb: […] Done https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/3173124d_1004b04e PS2, Line 1102: osmo_fd_write_disable(&state->upqueue.bfd); Ah alright, good to know (just read a bit into the code/`fd_set`-related matters). > This is wrong. You should disable the bfd after successful write() only if the queue is empty. In the core of the function body before my change, `bfd` was already disabled similarly - only if the queue wasn't empty - and before writing. That's what I meant by saying I didn't change the logic (as the loop becoming part of `osmo_wqueue_bfd_cb` was the main change). `osmo-bts at 495cf39cb9e6bb54a7a5fd1195988f4a03f3171a` on branch master: ``` while (!llist_empty(&state->upqueue)) { struct msgb *msg, *msg2; struct gsm_pcu_if *pcu_prim; /* peek at the beginning of the queue */ msg = llist_entry(state->upqueue.next, struct msgb, list); pcu_prim = (struct gsm_pcu_if *)msg->data; osmo_fd_write_disable(bfd); /* bug hunter 8-): maybe someone forgot msgb_put(...) ? */ if (!msgb_length(msg)) { LOGP(DPCU, LOGL_ERROR, "message type (%d) with ZERO " "bytes!\n", pcu_prim->msg_type); goto dontsend; } /* try to send it over the socket */ rc = write(bfd->fd, msgb_data(msg), msgb_length(msg)); if (rc == 0) goto close; if (rc < 0) { if (errno == EAGAIN) { osmo_fd_write_enable(bfd); break; } goto close; } dontsend: /* _after_ we send it, we can deueue */ msg2 = msgb_dequeue(&state->upqueue); assert(msg == msg2); msgb_free(msg); } return 0; ``` I think I've seen the same snippet in other socket write callbacks of ours, too. The enable happens in `pcu_sock_send()` or if `-EAGAIN` is received. https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/96912ed1_9864d2c1 PS2, Line 1181: osmo_wqueue_init(&state->upqueue, PCU_SOCK_QLENGTH_MAX_DEFAULT); > I'm fine with whatever but use WQUEUE instead of the QLENGTH stuff :) Done File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/16971e3b_72bf6e64 PS3, Line 993: LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %u messages waiting). Closing connection\n", > the code above is using DPCU everywhere, so I don't see a need to use a non-PCU-specific log subsyst […] Ah I see. I just went to the `LOGP` define and the comment above there doesn't mention that there are project-specific logging related headers like `include/osmocom/bsc/debug.h`. If `include/osmocom/<component>/debug.h` exists for different projects, it might be worth moving that code to libosmocore (seems like it's rather 'cross-component'). https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/3cdf00b0_e31e3f15 PS3, Line 993: LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %u messages waiting). Closing connection\n", > typo: than Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Gerrit-Change-Number: 31533 Gerrit-PatchSet: 4 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 02 Mar 2023 22:13:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bts[master]: common: Have PCU socket connection use osmo_wqueue
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: arehbein, pespin. Hello Jenkins Builder, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 to look at the new patch set (#4). Change subject: common: Have PCU socket connection use osmo_wqueue ...................................................................... common: Have PCU socket connection use osmo_wqueue Fixes memleak in case of connected PCU process being suspended without proper close on socket Related: OS#5774 Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 --- M include/osmo-bts/bts.h M include/osmo-bts/pcuif_proto.h M src/common/pcu_sock.c 3 files changed, 60 insertions(+), 67 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/33/31533/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Gerrit-Change-Number: 31533 Gerrit-PatchSet: 4 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in libosmocore[master]: osmo_wqueue_init(): Fix parameter type
by pespin 02 Mar '23

02 Mar '23

Attention is currently required from: arehbein, laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31595 ) Change subject: osmo_wqueue_init(): Fix parameter type ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Is it that bad potentially breaking some compilation with strict type checks for this? Seems to me l […] A simple typecast means all the potential users having to compile the app on their systems using some specific library version will suddenly make their compilation fail. Which means they have to find out why it doesn't compile, then make the change and even worse, try to find out how to make their app code work with both versions of the API. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31595 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I6c9295750e1755bf1d80f3a3b32efc2aefb11a57 Gerrit-Change-Number: 31595 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Thu, 02 Mar 2023 18:35:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: osmo_wqueue_init(): Fix parameter type
by arehbein 02 Mar '23

02 Mar '23

Attention is currently required from: laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31595 ) Change subject: osmo_wqueue_init(): Fix parameter type ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I meant "I am fine with either A and C". Is it that bad potentially breaking some compilation with strict type checks for this? Seems to me like it would always be an easy fix in this case (simple typecast?). -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31595 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I6c9295750e1755bf1d80f3a3b32efc2aefb11a57 Gerrit-Change-Number: 31595 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 02 Mar 2023 18:31:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0