Attention is currently required from: jolly, laforge, pespin, fixeria, dexter.

View Change

9 comments:

• Commit Message:

  • Patch Set #8, 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:

  • Patch Set #8, Line 207: "

    add "...is supported" or "support for ..."

  • Patch Set #8, Line 208: t

    same

• File src/common/l1sap.c:

  • Patch Set #7, 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:

  • Patch Set #8, 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?

  • Patch Set #8, 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?

  • Patch Set #8, 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.

  • Patch Set #8, 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?

  • Patch Set #8, Line 1969: break;

    the code converting from one format to the other should be in separate functions

To view, visit change 31417. To unsubscribe, or for help writing mail filters, visit settings.