Patch Set 3: (6 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) \ > phew, can we document the arguments? my head is spinning... At least now there's one parameter left storing state, so easier than before IMHO ;) Line 851: return ret; \ > wait, you are having a function return in this macro? I think that's bad st Why? this macro is intended to be used only in this set of functions, not to be used by other non-related code. It's just put here together because it's used in lots of places in a few lines. No need to clutter more parts of the functions with extra returns. return only happens if ret < 0 which means snprintf failed and in that case we just want to return that error to the caller, no need to do more work. Furthermore, it then simplifies the code (early return, no need to check specific code branches, as we can take some assumptions granted). Line 866: osmuxh->amr_ft, osmuxh->amr_cmr); > If I get this right, before this patch this function always returned 0 (off It could return other values depending on what was returned by ret. It's the same now, but also returning values <0 on error and checking overflows correctly, etc. 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 u As I said, I mostly rewrite the functions because there was bad logic + difficult to catch cases all around. Take it as a new implementation. Otherwise I'd need to spend hours and provide lots of commits to end up doing the same as you see here) or maybe worse. Not that worth spending more hours for this I think. Line 911: if (osmuxh->ft == OSMUX_FT_VOICE_AMR && !osmo_amr_ft_valid(osmuxh->amr_ft)) { > oof, is this change really related? It's related in the sense of improving the function. This check was missing because there are other types of osmux frames which can then end up in this error code patch and they should. I can split it into a new patch if you ask for it, but as I was already rewriting the function I decided to put it here too. Line 918: SNPRINTF_BUFFER_SIZE(ret, buf_offset, size); > (to illustrate above point, this macro call might return the function and b If there was an error in snprintf or if the buffer is full, why do I want to continue looping and wasting CPU time if anyway I cannot write into the buffer? I just want to return the error or return that we wrote the entire buffer. Why continuing? -- 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
