Patch Set 3: Code-Review-1 (5 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) \ > At least now there's one parameter left storing state, so easier than befor agreed; yet when first reading it, it helps to have doc for these non-obvious parameters. Line 851: return ret; \ > Why? this macro is intended to be used only in this set of functions, not t I strongly object this coding style. Please no hidden return statements in macros, it makes the code cryptic; the flow should be obvious to the uninformed, and that's worth a few more lines. IIUC I have even identified one instance below where it breaks the current code flow. Line 903: if (msg->len - msg_offset < sizeof(struct osmux_hdr)) { > As I said, I mostly rewrite the functions because there was bad logic + dif ok, but let's have a unit test Line 911: if (osmuxh->ft == OSMUX_FT_VOICE_AMR && !osmo_amr_ft_valid(osmuxh->amr_ft)) { > It's related in the sense of improving the function. This check was missing it is unrelated to range checking of snprintf calls. Should be separate IMHO. Line 918: SNPRINTF_BUFFER_SIZE(ret, buf_offset, size); > If there was an error in snprintf or if the buffer is full, why do I want t There was an error log statement ("No room for OSMUX payload") that you're now dropping. You shouldn't be failing silently. -- 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
