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.