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).