This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgPatch 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 <pespin at sysmocom.de> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger at freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-Reviewer: Pablo Neira Ayuso <pablo at gnumonks.org> Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de> Gerrit-HasComments: Yes