Patch Set 3: (4 comments)
https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c File src/osmux.c: Line 849: #define SNPRINTF_BUFFER_SIZE(ret, buffer_offset, size) \ > agreed; yet when first reading it, it helps to have doc for these non-obvio Agree, I'll add a short description of each parameter. Line 903: if (msg->len - msg_offset < sizeof(struct osmux_hdr)) { > ok, but let's have a unit test Agree Line 911: if (osmuxh->ft == OSMUX_FT_VOICE_AMR && !osmo_amr_ft_valid(osmuxh->amr_ft)) { > it is unrelated to range checking of snprintf calls. Should be separate IMH OK I'll split it Line 918: SNPRINTF_BUFFER_SIZE(ret, buf_offset, size); > There was an error log statement ("No room for OSMUX payload") that you're That's not checking the string output buffer, but the packet input buffer, it's different stuff. That check is there to prevent osmux_snprintf_payload() reading from put of bytes input buffer, and as our output buffer is full, there's no point in doing that check / print that error because anyway we are not interested in continuing and calling osmux_snprintf_payload(). So the early return imho is correct. -- To view, visit https://gerrit.osmocom.org/3537 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7 Gerrit-PatchSet: 3 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Holger Freyther <hol...@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pablo Neira Ayuso <pa...@gnumonks.org> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-HasComments: Yes
