Change in libosmocore[master]: LCLS: add string dump helper

[Neels Hofmeyr]

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