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