Change in libosmocore[master]: IPA: move duplicated error handling into function

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.org
Mon Dec 3 18:10:47 UTC 2018


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 <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-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 03 Dec 2018 18:10:47 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181203/99eddda0/attachment.htm>


More information about the gerrit-log mailing list