<p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_threads.h">File include/osmocom/mgcp/mgcp_threads.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_threads.h@90">Patch Set #27, Line 90:</a> <code style="font-family:monospace,monospace">         struct tapmsg {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think 'tap' and 'freep' require some kind of explanation comment. cfg/vty/loop is obvious to me.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h">File include/osmocom/mgcp/mgcp_trunk.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h@30">Patch Set #27, Line 30:</a> <code style="font-family:monospace,monospace">           struct per_thread_info *this_thread_info;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h@65">Patch Set #27, Line 65:</a> <code style="font-family:monospace,monospace">      unsigned int number_endpoints_offset;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">comment required.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c">File src/libosmo-mgcp/mgcp_protocol.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@a1402">Patch Set #27, Line 1402:</a> <code style="font-family:monospace,monospace">     * that we walk over all endpoints on the trunk in order to drop all</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">likewise with this move?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@324">Patch Set #27, Line 324:</a> <code style="font-family:monospace,monospace">#define rq w->x.rq</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@1188">Patch Set #27, Line 1188:</a> <code style="font-family:monospace,monospace">  if (!endp || !mgcp_endp_avail(endp)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">is moving this around really related to multithreading, or is it an unrelated change that should come before the multithread patch?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@1576">Patch Set #27, Line 1576:</a> <code style="font-family:monospace,monospace">    for_each_line (line, rq->pdatap->save) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">unrelated whitespace change</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432">change 25432</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I31be8253600c8af0a43c967d0d128f5ba7b16260 </div>
<div style="display:none"> Gerrit-Change-Number: 25432 </div>
<div style="display:none"> Gerrit-PatchSet: 27 </div>
<div style="display:none"> Gerrit-Owner: Hoernchen <ewild@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 12 Oct 2021 16:42:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>