Attention is currently required from: osmith, msuraev.

View Change

6 comments:

• Commit Message:

  • Patch Set #28, 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:

  • Patch Set #28, Line 51: + * osmo_fd to properly check for errors.

    you have diff '+' leaked into the C code

  • Patch Set #28, 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.

  • Patch Set #28, 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:

  • Patch Set #28, 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_documentation#Summary

  • Patch Set #28, 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)

To view, visit change 28848. To unsubscribe, or for help writing mail filters, visit settings.