libosmo-netif[master]: osmux: Re-write osmux_snprintf

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.org
Sat Sep 2 18:13:01 UTC 2017


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 <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



More information about the gerrit-log mailing list