Attention is currently required from: msuraev.

View Change

17 comments:

• File src/libsmpputil/smpp_msc.c:

  • Patch Set #49, Line 847: if (!g_smsc->link) {

    why is this check added now?

• File src/libsmpputil/smpp_smsc.c:

  • Patch Set #49, Line 166: acl->esme = NULL;

    Nice bug here setting to null before calling put on it. Better submit a patch preior to this one just fixing this.

  • Patch Set #49, Line 238: if (esme->use < 0)

    how can use be negative?

  • Patch Set #49, Line 347: osmo_stream_srv_send(esme->srv, msg);

    not return value to check?

  • Patch Set #49, Line 760: uint32_t smpp_size = esme->read_msg ? esme->read_msg->cb[0] : 0;

    you are casting unsigned long to "uint32_t". Better keep unsigned long?

  • Patch Set #49, Line 772: rc = osmo_stream_srv_recv(conn, esme->read_msg); /* N. B: the data will be appended to previously received (if any) */

    move the commend in the line above better.

  • Patch Set #49, Line 775: msgb_reset(esme->read_msg);

    does msgb_reset also clear esme->read_msg->cb[0]? we need to ensure it is 0 here.

  • Patch Set #49, Line 791: return -EBADF;

    why are you returning -EBADF here if the fd is not closed? This is wrong.

  • Patch Set #49, Line 799: /* check that we can receive expected amount of data */

    This whole block can be moved under the "esme->read_msg->cb[0] = smpp_size;" statement above, then it is only checked once instead of each time we receive data and the code is clear.

  • Patch Set #49, Line 803: LOGPESME(esme, LOGL_ERROR, "unable to reallocate %u bytes for message buffer\n", smpp_size);

    unable to resize message buffer to %u bytes.

  • Patch Set #49, Line 825: static int esme_link_read_cb(struct osmo_stream_srv *conn)

    Do we really need a second level callback function? Why not merging this function and the one above and have a goto path for the error paths as we usually do?
    I was first confused because you were returning -EBADF in the read callback function above.

  • Patch Set #49, Line 843: default:

    wrong indentation.

  • Patch Set #49, Line 851: static int esme_link_close_cb(struct osmo_stream_srv *conn)

    I foresee some problems with this and esme_destroy. I'll look at it closly when you fix the other stuff.

  • Patch Set #49, Line 949: osmo_stream_srv_link_set_addr(smsc->link, bind_addr ? bind_addr : "0.0.0.0");

    This should be improve in a follow up patch to probably pass NULL instead of "0.0.0.0" in order to support IPv6.

  • Patch Set #49, Line 985: osmo_stream_srv_link_close(smsc->link);

    you most probably want to set sms->link=NULL here from code I saw above.

• File src/libsmpputil/smpp_vty.c:

  • Patch Set #49, Line 83: bool is_running = smsc->link;

    Idea: You can probably add an smsc_is_running() API or whatever which checks this internally.

  • Patch Set #49, Line 190: vty_out(vty, " local-tcp-port %u%s", smsc->listen_port ? smsc->listen_port : SMPP_PORT, VTY_NEWLINE);

    I wonder why do we want to have listen_port set to 0 at any time, why not set it to SMPP_PORT by default?

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