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.orgNeels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12492 ) Change subject: LCLS: add string dump helper ...................................................................... Patch Set 7: Code-Review-1 (5 comments) https://gerrit.osmocom.org/#/c/12492/7/src/gsm/gsm0808_utils.c File src/gsm/gsm0808_utils.c: https://gerrit.osmocom.org/#/c/12492/7/src/gsm/gsm0808_utils.c@614 PS7, Line 614: * \returns string representation of LCLS or NULL on error */ missing crucial information: a *talloc* allocated string that the caller must free. I've done this type of thing before, and most of the time it turns out to be impractical. Instead I'd suggest returning a static string buffer that doesn't need freeing. Drawback is the fixed length, but you avoid leaking strings and you can guarantee a non-NULL return arg. Hence can directly include in printf() args. https://gerrit.osmocom.org/#/c/12492/7/src/gsm/gsm0808_utils.c@618 PS7, Line 618: #if (!EMBEDDED) Seeing this for the first time. The effect is that callers on embedded arch end up trying to print a NULL pointer? :P https://gerrit.osmocom.org/#/c/12492/7/src/gsm/gsm0808_utils.c@624 PS7, Line 624: return talloc_asprintf_append(s, ", %s", osmo_gcr_dump(lcls->gcr)); you can do these two in one printf call: "...%s%s", ... , print_gcr? ", " : "", print_gcr ? osmo_gcr_dump() : "" (especially useful if you switch to snprintf) In this current form, this is leaking the talloc string from osmo_gcr_dump(). Of course will be cleaned up as soon as the struct osmo_lcls is deallocated, but not so nice nevertheless. https://gerrit.osmocom.org/#/c/12492/7/src/gsm/gsm29205.c File src/gsm/gsm29205.c: https://gerrit.osmocom.org/#/c/12492/7/src/gsm/gsm29205.c@107 PS7, Line 107: osmo_talloc_asprintf(gcr, s, "Node 0x%x, CallRefID 0x%s", gcr->node, osmo_hexdump_nospc(gcr->cr, 5)); (maybe we can move _osmo_hexdump() to public API as osmo_hexdump_buf()?) https://gerrit.osmocom.org/#/c/12492/7/tests/gsm0808/gsm0808_test.c File tests/gsm0808/gsm0808_test.c: https://gerrit.osmocom.org/#/c/12492/7/tests/gsm0808/gsm0808_test.c@746 PS7, Line 746: printf("\t%s\n", osmo_gcr_dump(lcls_out->gcr)); should dealloc the strings. Some of our unit tests fail from the address sanitizer noticing non-deallocated buffers. Ah, in this case probably it gets cleaned from the lcls_out deallocation... -- To view, visit https://gerrit.osmocom.org/12492 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3609224c8f3282d667e75f68bc20327e36eb9e6 Gerrit-Change-Number: 12492 Gerrit-PatchSet: 7 Gerrit-Owner: Max <msuraev at sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <msuraev at sysmocom.de> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-Comment-Date: Wed, 09 Jan 2019 16:55:42 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190109/ce7e8bf8/attachment.htm>