Attention is currently required from: msuraev. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/28849 )
Change subject: libosmonetify SMPP ...................................................................... Patch Set 49: (17 comments) File src/libsmpputil/smpp_msc.c: https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/7ebe887b_ad124956 PS49, Line 847: if (!g_smsc->link) { why is this check added now? File src/libsmpputil/smpp_smsc.c: https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/17cecaf8_e7993d7b PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/827d4d94_8a9fb4fe PS49, Line 238: if (esme->use < 0) how can use be negative? https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/f2536112_0b0eb65f PS49, Line 347: osmo_stream_srv_send(esme->srv, msg); not return value to check? https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/b572a5a8_fa3652fe PS49, 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? https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/13927ea8_da495010 PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/4f744e45_a7e2c84e PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/17cf7e74_3916a0a6 PS49, Line 791: return -EBADF; why are you returning -EBADF here if the fd is not closed? This is wrong. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/ccc7f6e3_f2d1eb1a PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/9a159fd7_4a7055a7 PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/fddefff1_066f7386 PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/86f1073a_4fa4d24e PS49, Line 843: default: wrong indentation. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/d6084949_932b6847 PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/9fe1950c_2c4962e0 PS49, 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. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/98f9e91f_48ea9cfa PS49, 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: https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/1998658e_38c5f2af PS49, Line 83: bool is_running = smsc->link; Idea: You can probably add an smsc_is_running() API or whatever which checks this internally. https://gerrit.osmocom.org/c/osmo-msc/+/28849/comment/b1a08b27_fc25b615 PS49, 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 https://gerrit.osmocom.org/c/osmo-msc/+/28849 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: Idc2e60af1010783e555e61b114ae61f55a89d890 Gerrit-Change-Number: 28849 Gerrit-PatchSet: 49 Gerrit-Owner: msuraev <msur...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-Attention: msuraev <msur...@sysmocom.de> Gerrit-Comment-Date: Wed, 26 Oct 2022 16:39:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
