Attention is currently required from: osmith, msuraev. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/28848 )
Change subject: SMPP: move read/write callbacks to libsmpputil ......................................................................
Patch Set 28:
(6 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-msc/+/28848/comment/0f4c540b_64b8ea6a PS28, Line 7: SMPP: move read/write callbacks to libsmpputil would be good to give a reason for the move / which other caller is currently expected
File include/osmocom/smpp/smpp.h:
https://gerrit.osmocom.org/c/osmo-msc/+/28848/comment/dcbf9993_b50aaa3a PS28, Line 51: + * osmo_fd to properly check for errors. you have diff '+' leaked into the C code
https://gerrit.osmocom.org/c/osmo-msc/+/28848/comment/683ef905_fe48acca PS28, Line 53: + * an error. The code there should handle closing the connection. I would prefer to keep this macro confined locally to a .c file, i guess smpp_utils.c
- contains flow control (not local to a .c file but in a header file) - requiring a label - requiring a specific return type in calling code
If this makes it into public API, it needs a more detailed explanation, particularly about the 'return 0' meaning "try again". I guess best would be a code example of what the macro expects.
https://gerrit.osmocom.org/c/osmo-msc/+/28848/comment/95e06b96_0280d326 PS28, Line 55: + * projects as well. hmm idk.
this should have a name that is different from the general libosmocore-style to avoid naming conflicts. For example if you plan to move it to libosmocore, then you can clearly switch from one to the other. like SMPP_CHECK_FD_READ
File src/libsmpputil/smpp_utils.c:
https://gerrit.osmocom.org/c/osmo-msc/+/28848/comment/75c3dae7_8579125f PS28, Line 58: /* !\brief call-back when per-ESME TCP socket has some data to be read */ drop \brief, see https://osmocom.org/projects/cellular-infrastructure/wiki/Guidelines_for_API...
https://gerrit.osmocom.org/c/osmo-msc/+/28848/comment/8448af0d_bece25b5 PS28, Line 117: int esme_write_callback(struct esme *esme, int fd, struct msgb *msg) documentation for args and rc? (I tried to understand the 'exit(99)' below and found no explanation for rc == 0)