Attention is currently required from: pespin. arehbein 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: (7 comments) File include/osmo-bts/pcuif_proto.h: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/e798b3f5_d70a3eaa 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 sh […] I would say this is rather consistent with https://gerrit.osmocom.org/c/osmo-bts/+/6981 , which put PCU_SOCK_DEFAULT there in the first place. File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/d906cb23_9a0cfe37 PS2, Line 988: if (osmo_wqueue_enqueue(&state->upqueue, msg) == -ENOSPC) { > Simply check for any error "< 0" Done https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/c90c5a50_b825bf92 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? Yeah it's a bit confusing because `osmo_wqueue_init()` takes a regular `int` for I suppose historical reasons (I pushed a patch for that), so that's what I remembered last. Format specifier should be fixed now https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/10ff445f_2b2a210e PS2, Line 992: } else { > Usually we do early return on error, then drop this "else" block. Done https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/79fa212c_7c519e51 PS2, Line 1092: return -1; > In heere probably -EBADF needs to be returned so that the wqueue potentially > calls the write_cb() wi […] It doesn't change the control flow afaik (I looked at `osmo_wqueue_bfd_cb()`). But I could check on the errno stuff in these callbacks and improve on previously existing code https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/d1139b96_f34845bf 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. >From my understanding/by looking at this code, it looks like >`osmo_fd_write_disable()` is meant to prevent concurrent fd write-access via >osmo-wqueue code; with that in mind it would make sense to disable before >write. What has changed? I didn't change the logic of this function. https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/242b7e4a_e343f70f PS2, Line 1181: osmo_wqueue_init(&state->upqueue, PCU_SOCK_QLENGTH_MAX_DEFAULT); > I would split this into 2 commits if possible: […] No it's not possible, with the current code the current length of the wqueue is always incremented/there's always a length check. I find `PCU_SOCK_WQUEUE_LEN` misleading, because I would say the queue length is a property subject to change, so maybe `PCU_SOCK_WQUEUE_MAX_LEN` (I would also prefer not omitting the `_DEFAULT` suffix). Or maybe `PCU_SOCK_WQ_LEN_MAX_DEFAULT` ? I don't find `WQ` that absurd considering it's used right around `osmo_wqueue`-identifiers. -- 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: pespin <[email protected]> Gerrit-Comment-Date: Tue, 28 Feb 2023 12:54:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
