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/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
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 <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