Attention is currently required from: laforge, pespin, fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31341 )
Change subject: pcu_l1_if_phy: flexible phy access ...................................................................... Patch Set 11: (11 comments) Patchset: PS11: how does it work that only one phy ops entry is added? ...by only linking one of the phy implementation .c files, right? File src/osmobts_sock.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b57d8d16_a63bc618 PS11, Line 114: the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h); if (the_pcu->phy_ops->l1if_close_pdch) the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h); in general, cb functions may be left NULL if nothing should happen File src/pcu_l1_if.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/45fc38d9_d5047329 PS11, Line 218: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 0, fn, arfcn, block_nr, if (... check against NULL cb https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a55bd44c_0063a6a3 PS11, Line 247: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 1, fn, arfcn, block_nr, data, data_len); check NULL cb https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b6991b0a_1108bab9 PS11, Line 869: if (!bts->trx[trx_nr].fl1h) && cb != NULL File src/pcu_l1_if_phy.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/8a6cd8ba_2d317868 PS11, Line 2: * (C) 2023 by sysmocom s.f.m.c. GmbH <[email protected]> back in the days Holger asked me to add a dash sysmocom - s.f.m.c. GmbH https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/878685d1_413a8772 PS11, Line 51: l1if_phy_entry = talloc_zero(NULL, struct l1if_phy_entry); (i wonder if -fsanitize will report this as leaked memory whenever osmo-pcu exits .. i guess it would be better to either add a ctx arg to talloc the entry from, or to add a pcu_l1if_cleanup() that talloc_free()s all list entries that the main() calls before exiting) https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/450a8b1e_823b4167 PS11, Line 71: LOGPC(DLGLOBAL, LOGL_NOTICE, "\n"); please let's never ever use LOGPC() anymore! It was a messed up concept from the start, and in particular is very awkward in gsmtap_log. either a static buffer with OSMO_STRBUF_PRINTF() or char *msg = NULL; llist_for... { osmo_talloc_asprintf(OTC_SELECT, msg, "foo"); } LOGP(msg) and maybe add, at the start, if (!log_check_level(DLGLOBAL, LOGL_NOTICE)) return; https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b9a9e931_0aeefcdd PS11, Line 75: ops suggest name singleton_ops ...but seems to me the task this return arg solves could be achieved more elegantly? I'd like to see some code that acts on non-singleton phy lists, i guess will come up in another patch. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a3620cd4_a4ecaa24 PS11, Line 81: *ops = NULL; let's add /* make sure a singleton is not mixed with other PHY */ llist_for_each_entry(l1if_phy_entry, &phy_list, list) if (l1if_phy_entry->ops->singleton) OSMO_ASSERT(llist_count(&phy_list) == 1); without this check, below loop would initialize other non-singleton phy before exiting the list on the first singleton phy, and would skip any other entries ... i.e. act weird without complaining. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a8dfec3e_eb6928f0 PS11, Line 100: if (*ops) { so the indication whether a phy is in use depends on the argument that the caller passes in to be zero initialized? seems awkward to me / gives weird errors when forgetting to zero initialize. When i see non-singleton code i might have a suggestion for a more elegant solution... for now i can only complain =) -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31341 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I8692d1bd5d137a17cf596ee2914722f419c9978d Gerrit-Change-Number: 31341 Gerrit-PatchSet: 11 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-CC: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Attention: dexter <[email protected]> Gerrit-Comment-Date: Mon, 27 Feb 2023 00:32:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
