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: Code-Review-1 (13 comments) The implementation is almost perfect, but we need to improve on the API doc and unit tests... https://gerrit.osmocom.org/#/c/4200/3/src/msgb.c File src/msgb.c: Line 383: /*! Print a string to the end of message buffer Always end with a full stop '.' as I commented numerous times before. Besides being common sense, it's to guarantee the doxygen autobrief works as expected. Line 388: * will not be changed at all. If the buffer has no tailroom left (<1 means ==0), then there is no valid way to change the buffer, so this remark does not really say much beyond what is obvious. It would be important to focus on the nul terminator, maybe something like: The resulting string is printed to the msgb without a trailing nul character. A nul following the data tail may be written as an implementation detail, but a trailing nul is never part of the msgb data in terms of msgb_length(). Note: the tailroom must always be one byte longer than the string to be written. The msgb is filled only up to tailroom=1. This is an implementation detail that allows leaving a nul character behind the valid data. Line 390: * In case of error, the message buffer may be left with invalid data This should be: In case of error, the msgb remains unchanged, though data may have been written to the (unused) memory after the tail pointer. Does snprintf even write anything on error? I think not, right? In that case we can just write that it remains unchanged and done. Line 394: * ready for a second attempt (with less data). I think we don't need to hint at second attempts, it follows naturally from "remains unchanged". Line 399: * terminator. Doesn't this merely restate "Print a string to the end of a message buffer"? Drop this paragraph? Line 402: * result will be a single, concatenated string. Also obvious and can be dropped? Line 424: *msgb->tail = '\0'; hmm really? I would rather leave unchanged, and leave \0 responsibility with the caller; I don't want to assume that \0 is part of the protocol used by the caller. Also questionable whether tail still points within valid buffer space. Let's rather drop this line? https://gerrit.osmocom.org/#/c/4200/3/tests/msgb/msgb_test.c File tests/msgb/msgb_test.c: Line 289: total_len = strlen((char *)msg->data); Rather use msgb_length() to verify what the msgb reflects as its length, not strlen() to interpret the data externally. With the current test you will not be able to catch erratic msgb_length(). The terminating nul is, to me, just an implementation detail, a side effect from snprintf() that we should not need nor rely on. (same each time below) Line 291: msg->data); hmm, this also relies on a terminating nul. We should again rather dump exactly the part marked as valid by msgb_length() ... not sure how to do it best, maybe with a local utility function that prints exactly msgb_length() single characters? Like that we would also catch a \0 char written in the middle of the valid data...? Line 300: below you assert tailroom == 17, I'd also have the same assert here, so that it is clear the tailroom was not changed by the call. Line 324: OSMO_ASSERT(total_len == 63); rather assert msgb_length() == 63 Line 333: /* Make sure that the string in the bufer takes up all available "all available space." ? And then start a new sentence with a captial letter? Line 349: OSMO_ASSERT(total_len == 79); and finally please add one test where an already full buffer with tailroom == 0 is passed to msgb_printf() and assert that nothing has changed. Best would be to also somehow assert that no \0 was written behind the memory allocated by the msgb. -- To view, visit https://gerrit.osmocom.org/4200 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15e1af68616309555d0ed9ac5da027c9833d42e3 Gerrit-PatchSet: 3 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: dexter <pmaier at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max <msuraev at sysmocom.de> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-HasComments: Yes