Attention is currently required from: jolly, laforge, pespin, fixeria, dexter.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31417 ) Change subject: l1sap: Accept RFC5993 and TS 101.318 HR GSM payload ...................................................................... Patch Set 8: (9 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/39f49542_c4452fbe PS8, Line 16: are accepted. I am not clear here, there is code in this patch that converts the HR format to match the BTS, and there is also code that rejects an RTP packet when the HR format does not match the BTS. AFAICT we should need only one of these. Could you please clarify the intention of this patch? File src/common/bts.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/72c3c35d_453b25eb PS8, Line 207: " add "...is supported" or "support for ..." https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/02db462e_9a563be5 PS8, Line 208: t same File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/8682887e_df3ca21b PS7, Line 1274: if (OSMO_UNLIKELY(rfc5993 && (resp_msg->len != GSM_HR_BYTES_RTP_RFC5993))) { > >> in case both or none of the flags are set both encodings are recognized as > >> valid formats. […] I agree that a flag being zero should disallow that format, no matter what the other flag's value is. I'd also ask Pau please to reflect on particular wording choices in your code review comments. We are aiming for a supportive atmosphere that invites developers. I'm pretty sure you are commenting with a friendly wink, but these easily go unnoticed in a written medium. Thanks! =) File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/815cffbb_2ae703df PS8, Line 1244: static bool rtppayload_is_valid(struct gsm_lchan *lchan, struct msgb *resp_msg) > if this is expected to be used only for incoming DL packets we forward to > lower layers towards the M […] AFAICT the function name is not part of this patch. you are welcome to submit your own patch to improve the naming? https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/881ccae6_9c20663f PS8, Line 1274: /* BTS flags specify that RFC 5993 (and not ETSI TS 101.318) is understood by lower layers */ > All this code block below you are writing can iiuc be simplified to: […] I agree with the patch here, to keep the logic of this function uniform: "return false if anything is wrong, and otherwise carry on to the final 'return true'". Possibly the incoming packet is not HR at all, and we only exit with false if it is HR and also the format is not supported. Also agree with the patch in adding concise logging. But I think as in earlier comments the conditions here should be if (len == GSM_HR_BYTES_FOO && !foo_supported) { LOG("dropping unsupported RTP payload format foo") return false; } (two of these, one for each format) EDIT: but I am confused about whether this patch is instead converting to another format, then this part here can be dropped? https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/385a8d55_9ddeec5a PS8, Line 1275: if (OSMO_UNLIKELY(rfc5993 && !ts101318 && (resp_msg->len != GSM_HR_BYTES_RTP_RFC5993))) { I'm still trying to understand OSMO_UNLIKELY() -- seems to me that this is not a good use case for OSMO_UNLIKELY()? if anyone can explain that would be welcome. https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/eda2b1d9_6bab0c0e PS8, Line 1952: * formats are supported (either by setting both or none of the flags), no conversion will be carried out. */ > we don't care about "both formats being supported". […] need clarification: is this patch converting or rejecting? https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/95b98f55_11997f15 PS8, Line 1969: break; the code converting from one format to the other should be in separate functions -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31417 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I453562da412fde5b928bd2b588129c58ec8e2a7e Gerrit-Change-Number: 31417 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: jolly <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: neels <[email protected]> Gerrit-Attention: jolly <[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: Thu, 04 May 2023 17:00:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: dexter <[email protected]> Gerrit-MessageType: comment
