Patch Set 3: (7 comments)
There's a lot going on in this patch, and it practically screams for a small unit test explicitly checking osmux_snprintf() behavior for "all" the edge cases. That would verify beyond doubt that you're not just shooting in the dark but actually fixing behavior. https://gerrit.osmocom.org/#/c/3537/3//COMMIT_MSG Commit Message: Line 12: to try harder at fixing those issues. lol, "attempts to try harder" -- you don't sound awfully certain of this patch? :) https://gerrit.osmocom.org/#/c/3537/3/src/osmux.c File src/osmux.c: Line 849: #define SNPRINTF_BUFFER_SIZE(ret, buffer_offset, size) \ phew, can we document the arguments? my head is spinning... Line 851: return ret; \ wait, you are having a function return in this macro? I think that's bad style. Rather assign a value to buf_offset and let the caller decide on the return value? Line 866: osmuxh->amr_ft, osmuxh->amr_cmr); If I get this right, before this patch this function always returned 0 (offset = 0 was never changed) and now it returns the amount of bytes printed? Looks sane and a fix by itself. Line 903: if (msg->len - msg_offset < sizeof(struct osmux_hdr)) { This change alone is complex enough. With the rest also being changed I'm unable to wrap my head around it easily. Line 911: if (osmuxh->ft == OSMUX_FT_VOICE_AMR && !osmo_amr_ft_valid(osmuxh->amr_ft)) { oof, is this change really related? Line 918: SNPRINTF_BUFFER_SIZE(ret, buf_offset, size); (to illustrate above point, this macro call might return the function and below logging and the remaining loops will just not happen. It makes the code much harder to read at the very least and is certainly not what you want to do here anyway.) -- 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 <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Holger Freyther <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pablo Neira Ayuso <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-HasComments: Yes
