Patch Set 2: Code-Review-1 (2 comments)
One thing that's worse after this patch: we add a couple of short-lived small dynamic allocations (gsup message, msisdn, apn). The root cause being that we failed to use actual arrays in the original definition of the gsup message struct. Instead of talloc, we could pass in non-dynamic buffers (maybe even in a struct definition with the sole purpose of providing uint8_t[] for a gsup message), or we could use static buffers within the new function (and make it non-threadsafe, which is ok because we use select() to handle messages one after the other anyway). What do others think, is avoiding dynamic allocation micro-optimisation or worth rewriting this patch for? https://gerrit.osmocom.org/#/c/7992/2/src/gsup_server.c File src/gsup_server.c: Line 369: gsup = talloc_zero(NULL, struct osmo_gsup_message); do not talloc from NULL ctx, pass a ctx in as arg. If you talloc at NULL, we are likely to miss memory leaks should they show up. Line 373: osmo_strlcpy(gsup->imsi, imsi, GSM23003_IMSI_MAX_DIGITS + 1); wouldn't it make sense to use sizeof(gsup->imsi) instead of guessing the same constants again, hopefully correctly? -- To view, visit https://gerrit.osmocom.org/7992 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a92ca34cdaadca9eacc774bb1ca386c325ba865 Gerrit-PatchSet: 2 Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Owner: Stefan Sperling <ssperl...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Stefan Sperling <ssperl...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
