This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/25432 )
Change subject: Add multithreading for the virtual trunk
......................................................................
Patch Set 27:
(7 comments)
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_threads.h
File include/osmocom/mgcp/mgcp_threads.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_threads.h@90
PS27, Line 90: struct tapmsg {
I think 'tap' and 'freep' require some kind of explanation comment. cfg/vty/loop is obvious to me.
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h
File include/osmocom/mgcp/mgcp_trunk.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h@30
PS27, Line 30: struct per_thread_info *this_thread_info;
what is the rationale of using a union here? Wasting 8 bytes for a pointer is not all that much. Having separate pointers would allow us to catch "wrong" dereferences, i.e. if a master trunk code accesses this_thread_info, it would get a NULL pointer and crash, instead of operating on "forbidden" dat structures. Likewise also the other way around: If a thread accesses thread_info, it would be NULL in a per-thread mgcp_trunk and thus crash?
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h@65
PS27, Line 65: unsigned int number_endpoints_offset;
comment required.
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@a1402
PS27, Line 1402: * that we walk over all endpoints on the trunk in order to drop all
likewise with this move?
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@324
PS27, Line 324: #define rq w->x.rq
not sure I'm a fan of this, writing those few more characters make it more cleare what is done in the code. Not a blocker, just my thinking.
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@1188
PS27, Line 1188: if (!endp || !mgcp_endp_avail(endp)) {
is moving this around really related to multithreading, or is it an unrelated change that should come before the multithread patch?
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@1576
PS27, Line 1576: for_each_line (line, rq->pdatap->save) {
unrelated whitespace change
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/25432
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I31be8253600c8af0a43c967d0d128f5ba7b16260
Gerrit-Change-Number: 25432
Gerrit-PatchSet: 27
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: dexter <pmaier at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Tue, 12 Oct 2021 16:42:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211012/42962b8f/attachment.htm>