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

[S] Change in libosmocore[master]: gsmtap: Hide implementation of gsmtap_inst
by pespin 12 Oct '23

12 Oct '23

Attention is currently required from: arehbein, daniel, laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email ) Change subject: gsmtap: Hide implementation of gsmtap_inst ...................................................................... Patch Set 2: (1 comment) File include/osmocom/core/gsmtap_util.h: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/f2bed0d0_97a59349 PS2, Line 30: > I think this would allow us to keep `gsmtap_inst_fd()` as `static inline` inside the header, without […] No, better move it to a function like you did here. Already compiled code will already have the implementation compiled in its code, but at least for new code we now control the implementation in the lib. The point of having gsmtap_inst_fd2() as separate of gsmtap_inst_fd2(), is that we can mark the former as DEPRECATED and force everybody to update to gsmtap_inst_fd2() at some point, meaning we know for sure all users will end up using the non-static-inline version. So the change of static inline to a full function can be tracked more easily and more clearly. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibe1a51205a6df764571b6d074e365825555609a5 Gerrit-Change-Number: 34720 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 13:42:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsmtap: Hide implementation of gsmtap_inst
by pespin 12 Oct '23

12 Oct '23

Attention is currently required from: arehbein, daniel, laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email ) Change subject: gsmtap: Hide implementation of gsmtap_inst ...................................................................... Patch Set 2: (1 comment) File src/core/gsmtap_util.c: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/ca876c4c_19f0a5f4 PS2, Line 52: struct osmo_wqueue wq; /*!< the wait queue */ > How about also adding the same struct decl. with another tag, e.g. […] Sounds good to me, but have a look at osmo_static_assert instead of OSMO_ASSERT. That one is checked at compile time once instead of every time the code is executed. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibe1a51205a6df764571b6d074e365825555609a5 Gerrit-Change-Number: 34720 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 13:36:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in docker-playground[master]: debian-bookworm-titan: add ccache
by pespin 12 Oct '23

12 Oct '23

Attention is currently required from: osmith. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/docker-playground/+/34703?usp=email ) Change subject: debian-bookworm-titan: add ccache ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/docker-playground/+/34703?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: docker-playground Gerrit-Branch: master Gerrit-Change-Id: I94d22b8da9f897974c5913b2a8138c653c215446 Gerrit-Change-Number: 34703 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 13:35:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsmtap: Hide implementation of gsmtap_inst
by arehbein 12 Oct '23

12 Oct '23

Attention is currently required from: daniel, laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email ) Change subject: gsmtap: Hide implementation of gsmtap_inst ...................................................................... Patch Set 2: (1 comment) File include/osmocom/core/gsmtap_util.h: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/32b7b9d8_6b53441d PS2, Line 30: > Instead of adding `gsmtap_inst_fd2()`, I could also just keep using `wq.bfd. […] I think this would allow us to keep `gsmtap_inst_fd()` as `static inline` inside the header, without needing to change that part of the API? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibe1a51205a6df764571b6d074e365825555609a5 Gerrit-Change-Number: 34720 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 13:29:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsmtap: Hide implementation of gsmtap_inst
by arehbein 12 Oct '23

12 Oct '23

Attention is currently required from: daniel, laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email ) Change subject: gsmtap: Hide implementation of gsmtap_inst ...................................................................... Patch Set 2: (1 comment) File include/osmocom/core/gsmtap_util.h: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/5164a842_f5a40261 PS2, Line 30: > I think it may be a good idea to add the new symbol as gsmtap_inst_fd2() and change the known client […] Instead of adding `gsmtap_inst_fd2()`, I could also just keep using `wq.bfd.fd` through the `wq` member of `struct gsm_inst` for Osmo IO, only acessing it through `gsmtap_inst_fd()` even internally. That should work, because Osmo IO will replace usage of the write queue altogether. And if by 'changing the internal structure', you meant getting rid of the `wq` struct member altogether and adding, say a member `int fd` internally, that can also easily be done then later. And we avoid adding an additional API function. Any objections to that? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibe1a51205a6df764571b6d074e365825555609a5 Gerrit-Change-Number: 34720 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 13:27:05 +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 docker-playground[master]: debian-bookworm-titan: add ccache
by osmith 12 Oct '23

12 Oct '23

Attention is currently required from: osmith, pespin. Hello Jenkins Builder, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/docker-playground/+/34703?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review+2 by pespin, Verified+1 by Jenkins Builder The change is no longer submittable: Code-Review and Verified are unsatisfied now. Change subject: debian-bookworm-titan: add ccache ...................................................................... debian-bookworm-titan: add ccache Add ccache, so it can be used when rebuilding the testsuite from source from a development branch. Ccache is not used by default. Related: osmo-dev I800062d0379295a6905851db29e820ff16217653 Change-Id: I94d22b8da9f897974c5913b2a8138c653c215446 --- M debian-bookworm-titan/Dockerfile 1 file changed, 18 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/docker-playground refs/changes/03/34703/2 -- To view, visit https://gerrit.osmocom.org/c/docker-playground/+/34703?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: docker-playground Gerrit-Branch: master Gerrit-Change-Id: I94d22b8da9f897974c5913b2a8138c653c215446 Gerrit-Change-Number: 34703 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in libosmocore[master]: gsmtap: Hide implementation of gsmtap_inst
by arehbein 12 Oct '23

12 Oct '23

Attention is currently required from: daniel, laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email ) Change subject: gsmtap: Hide implementation of gsmtap_inst ...................................................................... Patch Set 2: (1 comment) File src/core/gsmtap_util.c: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/e5224a24_4a583741 PS2, Line 52: struct osmo_wqueue wq; /*!< the wait queue */ > This is fine because it keeps the same offset. […] How about also adding the same struct decl. with another tag, e.g. `struct gsmtap_inst_legacy` (same members etc.), to fix the old struct and then add `OSMO_ASSERT(offsetof(struct gsmtap_inst_legacy, wq) == offsetof(struct gsmtap_inst, wq))` when running `gsmtap_source_init*()`? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibe1a51205a6df764571b6d074e365825555609a5 Gerrit-Change-Number: 34720 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 13:13:40 +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 docker-playground[master]: debian-bookworm-titan: add ccache
by pespin 12 Oct '23

12 Oct '23

Attention is currently required from: osmith. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/docker-playground/+/34703?usp=email ) Change subject: debian-bookworm-titan: add ccache ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/docker-playground/+/34703?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: docker-playground Gerrit-Branch: master Gerrit-Change-Id: I94d22b8da9f897974c5913b2a8138c653c215446 Gerrit-Change-Number: 34703 Gerrit-PatchSet: 1 Gerrit-Owner: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 12:46:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsmtap: Hide implementation of gsmtap_inst
by pespin 12 Oct '23

12 Oct '23

Attention is currently required from: arehbein, daniel, laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email ) Change subject: gsmtap: Hide implementation of gsmtap_inst ...................................................................... Patch Set 2: (4 comments) File include/osmocom/core/gsmtap_util.h: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/c4955b0a_53fe1e2e PS2, Line 29: int gsmtap_inst_fd(struct gsmtap_inst *gti); This API being static inline before, means the appsusing it actually got the implementation compiled in in their code. In detail, these ones: """ osmocom-bb/src/shared/libosmocore/src/gsmtap_util.c 217: rc = write(gsmtap_inst_fd(gti), msg->data, msg->len); 305: fd = gsmtap_source_add_sink_fd(gsmtap_inst_fd(gti)); osmo-remsim/src/bankd/gsmtap.c 100: const int rc = writev(gsmtap_inst_fd(g_gti), iov, cnt); """ This means these apps are actually aware and using/calculating the place of the fields inside the struct. Hence, it's fine moving the struct to be private, but you must keep the same position of wq.bfd.fd within the struct (and fd must keep containing the good value). https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/6ea6dd7b_1685edc7 PS2, Line 30: I think it may be a good idea to add the new symbol as gsmtap_inst_fd2() and change the known clients to use that symbol, so at least we control at which time the ABI issue with new apps is gone (and let's say, accept the internal structure can be changed in 2-5 years). File src/core/gsmtap_util.c: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/e6b53401_e07124d8 PS2, Line 52: struct osmo_wqueue wq; /*!< the wait queue */ This is fine because it keeps the same offset. However, you lack adding a big fat commit that this wq must remain in the exact same position forever. File src/core/libosmocore.map: https://gerrit.osmocom.org/c/libosmocore/+/34720/comment/02ec06be_8e25f481 PS2, Line 42: gsmtap_inst_fd; > Not sure if this needs a line in TODO-RELEASE? It seems like an addition to the ABI, even if it was […] Yes, but let's add it as gsmtap_inst_fd2 better. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34720?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibe1a51205a6df764571b6d074e365825555609a5 Gerrit-Change-Number: 34720 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 12:26:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: daniel <dwillmann(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-ttcn3-hacks[master]: hss: Test Operator-Determined-Barring feature
by osmith 12 Oct '23

12 Oct '23

Attention is currently required from: pespin. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/34718?usp=email ) Change subject: hss: Test Operator-Determined-Barring feature ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/34718?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ttcn3-hacks Gerrit-Branch: master Gerrit-Change-Id: I8deea9e284447f8991bdeced982c14fae2f211c5 Gerrit-Change-Number: 34718 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 12 Oct 2023 08:00:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0