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

Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Mon Sep 4 12:58:48 UTC 2017


Patch Set 3:

(4 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)		\
> agreed; yet when first reading it, it helps to have doc for these non-obvio
Agree, I'll add a short description of each parameter.


Line 903: 		if (msg->len - msg_offset < sizeof(struct osmux_hdr)) {
> ok, but let's have a unit test
Agree


Line 911: 		if (osmuxh->ft == OSMUX_FT_VOICE_AMR && !osmo_amr_ft_valid(osmuxh->amr_ft)) {
> it is unrelated to range checking of snprintf calls. Should be separate IMH
OK I'll split it


Line 918: 		SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
> There was an error log statement ("No room for OSMUX payload") that you're 
That's not checking the string output buffer, but the packet input buffer, it's different stuff.

That check is there to prevent osmux_snprintf_payload() reading from put of bytes input buffer, and as our output buffer is full, there's no point in doing that check / print that error because anyway we are not interested in continuing and calling osmux_snprintf_payload(). So the early return imho is correct.


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