Attention is currently required from: jtavares. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-remsim/+/30139 ) Change subject: bankd: Add GSMTAP functionality for SIM traffic ...................................................................... Patch Set 2: (5 comments) Patchset: PS2: In general I'd like to see this feature included, just some minor stuff, see comments. File src/bankd/gsmtap.c: https://gerrit.osmocom.org/c/osmo-remsim/+/30139/comment/5090e9b3_1e1d2f79 PS2, Line 28: osmo_bankd_gsmtap_init there's a problem with the naming prefix, sorry. In general, we reserve the osmo_ prefix of global/exported symbols to our libraries like libosmo{core,abis,gsm,netif,sccp,sigtran,mgcp,...}. Applications should generally not use this prefix to avoid potential future namespace collissions with library functions introduced at a later point. See also https://osmocom.org/projects/cellular-infrastructure/wiki/Coding_standards#… https://gerrit.osmocom.org/c/osmo-remsim/+/30139/comment/07aac0ee_c5a3ea69 PS2, Line 35: perror("unable to open GSMTAP"); normally we'd want to use the libosmocore logging framework with LOGL_ERROR here. https://gerrit.osmocom.org/c/osmo-remsim/+/30139/comment/c387ce7e_dc36944b PS2, Line 56: uint8_t *buf = malloc(gross_len); not critical, just a comment: we could do without a dynamic heap allocation + memcpy here if we were to use writev() instad of write(). This way we could have the header on the stack, and an iovec[3] for header, mdm-tpdu and sim-tpdu. It's not critical as this is not a performance critical code path. https://gerrit.osmocom.org/c/osmo-remsim/+/30139/comment/e54d931c_c9643199 PS2, Line 83: perr normally we'd want to use the libosmocore logging framework with LOGL_ERROR here. -- To view, visit https://gerrit.osmocom.org/c/osmo-remsim/+/30139 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-remsim Gerrit-Branch: master Gerrit-Change-Id: I05b599858d8758633aa56c3f12f258c27cf42d08 Gerrit-Change-Number: 30139 Gerrit-PatchSet: 2 Gerrit-Owner: jtavares <jtavares(a)kvh.com> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: jtavares <jtavares(a)kvh.com> Gerrit-Comment-Date: Mon, 14 Nov 2022 17:12:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment