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