Attention is currently required from: arehbein. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31533 )
Change subject: common: Have PCU socket connection use osmo_wqueue ...................................................................... Patch Set 2: Code-Review-1 (7 comments) File include/osmo-bts/pcuif_proto.h: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/02ac26ec_654e5dfe PS2, Line 8: #define PCU_SOCK_QLENGTH_MAX_DEFAULT 10 that's application/peer implementation specific, not really part of the shared protocol and hence shouldn't be placed here. Bear in mind this file is duplicated in 3 different project, so avoid adding protocol-unneeded stuff in here. File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/e0ac2792_f4e2a60b PS2, Line 988: if (osmo_wqueue_enqueue(&state->upqueue, msg) == -ENOSPC) { Simply check for any error "< 0" https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/9145aecb_4bbcd0c5 PS2, Line 989: LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %d messages waiting). Closing connection\n", max_length is an int? or an unsigned? https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/275826ba_ecc9927d PS2, Line 992: } else { Usually we do early return on error, then drop this "else" block. https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/c58648fd_4899f0a9 PS2, Line 1092: return -1; In heere probably -EBADF needs to be returned so that the wqueue potentially calls the write_cb() with a closed/freed socket. https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/c46f42b9_b556ea96 PS2, Line 1102: osmo_fd_write_disable(&state->upqueue.bfd); This is wrong. You should disable the bfd after successful write() only if the queue is empty. https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/84533b8f_392f737f PS2, Line 1181: osmo_wqueue_init(&state->upqueue, PCU_SOCK_QLENGTH_MAX_DEFAULT); I would split this into 2 commits if possible: 1 commit changing the implementation to wqueue but keeping unlimited wqueue size (is that possible by passin 0 here?) 1 commit afterwards changing the queue size to PCU_SOCK_QLENGTH_MAX_DEFAULT. btw, PCU_SOCK_WQUEUE_LEN it's more clear imho (had to think hat QLENGTH meant). -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Gerrit-Change-Number: 31533 Gerrit-PatchSet: 2 Gerrit-Owner: arehbein <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: arehbein <[email protected]> Gerrit-Comment-Date: Mon, 27 Feb 2023 12:22:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
