Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11911 )
Change subject: IPA: move duplicated error handling into function ...................................................................... Patch Set 4: Code-Review-1 (3 comments) I find the choice of name and return value a bit confusing: - recv_discard_msg() reads like it actually calls msgb_free() implicitly, but we still 'goto discard_msg', looks like we discard twice? - the return value of 'true' would to me intuitively mean "reading was successful". Trying to come up with a nicer API it seems to me that it's not worth cracking our head on for those mere two cases below, especially since there is still more return value checking in 657: 'if (ret < needed)' -- it makes it harder to read what is actually going on. So I appreciate the code dup removal, but I believe in this instance it comes at a cost, which is too high (both readability wise and time spent wise). (some more comments follow from before I reached that conclusion) https://gerrit.osmocom.org/#/c/11911/4//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/#/c/11911/4//COMMIT_MSG@10 PS4, Line 10: we do it in oter places. 'h' key stuck? :) https://gerrit.osmocom.org/#/c/11911/4/src/gsm/ipa.c File src/gsm/ipa.c: https://gerrit.osmocom.org/#/c/11911/4/src/gsm/ipa.c@548 PS4, Line 548: * \param[in] needed Home many bytes we have to read. 'How'? Actually, please rather just use the recv() arg name and refer there, as it is passed transparently: size_t len len parameter passed to recv() https://gerrit.osmocom.org/#/c/11911/4/src/gsm/ipa.c@553 PS4, Line 553: *ret = recv(fd, msg->tail, needed, 0); This looks like it lacks some bounds checking: make sure that msgb has enough room for 'needed' at its tail? (Or do all callers cover that already?) Would it make sense to derive the 'needed' (len) parameter from the msgb? (I guess not) -- To view, visit https://gerrit.osmocom.org/11911 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: Ic0147bffaf04b0baf97e5cca22948bd0e116668f Gerrit-Change-Number: 11911 Gerrit-PatchSet: 4 Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Mon, 03 Dec 2018 18:10:47 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
