Attention is currently required from: pespin.
7 comments:
File include/osmo-bts/pcuif_proto.h:
Patch Set #2, 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:
Patch Set #2, Line 988: if (osmo_wqueue_enqueue(&state->upqueue, msg) == -ENOSPC) {
Simply check for any error "< 0"
Done
Patch Set #2, 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
Patch Set #2, Line 992: } else {
Usually we do early return on error, then drop this "else" block.
Done
Patch Set #2, 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
Patch Set #2, 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.
Patch Set #2, 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 change 31533. To unsubscribe, or for help writing mail filters, visit settings.