<p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432">View Change</a></p><p>20 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/32/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/32/include/osmocom/mgcp/mgcp_threads.h@73">Patch Set #32, Line 73:</a> <code style="font-family:monospace,monospace">struct to_trunkthread_cfg_msg {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This "to" prefix here is very confusing imho.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/include/osmocom/mgcp/mgcp_threads.h@115">Patch Set #32, Line 115:</a> <code style="font-family:monospace,monospace">struct per_thread_info {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">imho this "per_" prefix can also be dropped, it confuses more than helps.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/include/osmocom/mgcp/mgcp_threads.h@121">Patch Set #32, Line 121:</a> <code style="font-family:monospace,monospace">       int tid; /* thread number handling this subtrunk */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">as in gettid()? or some posix threads id?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">as in "thread number handling this subtrunk"</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/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/32/src/libosmo-mgcp/mgcp_protocol.c@319">Patch Set #32, Line 319:</a> <code style="font-family:monospace,monospace">   ssize_t rc = w->x.msglen;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this looks a bit weird, assigning a length to rc like this.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_protocol.c@326">Patch Set #32, Line 326:</a> <code style="font-family:monospace,monospace">    if (rc < sizeof(rq->name) - 1) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">afaict you are only using rc here and in line 333 before setting it below, may be just more understa […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_protocol.c@331">Patch Set #32, Line 331:</a> <code style="font-family:monospace,monospace">   memcpy(rq->name, (const char *)&w->msg[0], sizeof(rq->name) - 1);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no null char at the end of rq->name is required?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this copies the 4 byte string into a 5 byte array in a zero-initialized struct.</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/32/src/libosmo-mgcp/mgcp_protocol.c@396">Patch Set #32, Line 396:</a> <code style="font-family:monospace,monospace">                                       ti->dlcx_in_queue++;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what's the relation between dlc_in_queue and having free endpoints? I cannot see the relation at fir […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">it might be not empty, and there might be other crcx in it that will be processed first, so just ignoring it means we could accidentally bother a thread that has no free eps after all.</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/32/src/libosmo-mgcp/mgcp_protocol.c@407">Patch Set #32, Line 407:</a> <code style="font-family:monospace,monospace">                  return NULL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Doesn't "w" need to be freed in any of these paths?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">no, w lives on the caller stack, and is copied to the queue, so always safe.</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/32/src/libosmo-mgcp/mgcp_protocol.c@1531">Patch Set #32, Line 1531:</a> <code style="font-family:monospace,monospace">            * endpoints, possible open connections are forcefully dropped */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">need to fix formatting here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c">File src/libosmo-mgcp/mgcp_threads.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/32/src/libosmo-mgcp/mgcp_threads.c@44">Patch Set #32, Line 44:</a> <code style="font-family:monospace,monospace">#define GETF(yy, zz)                                                                                                   \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">That's 10 lines for all the macro + definition stuff. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Macros are safe and idiot proof and it's not my fault that we enforce kernel style formatting which takes up a lot of space, I'm not going to make code difficult to work with and a copy-paste trap just because formatting makes this look ugly.</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/32/src/libosmo-mgcp/mgcp_threads.c@337">Patch Set #32, Line 337:</a> <code style="font-family:monospace,monospace">void *split_per_thead(void *info)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">THis function name is misleading. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@344">Patch Set #32, Line 344:</a> <code style="font-family:monospace,monospace">     top_ctx = talloc_named_const(NULL, 0, "top_thread_ctx");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">idea: maybe add this_thread_number to the string here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@357">Patch Set #32, Line 357:</a> <code style="font-family:monospace,monospace">    struct mgcp_trunk *t = thread_info->this_trunk;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please move this to the start, I was not finding it when looking for "t".</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@369">Patch Set #32, Line 369:</a> <code style="font-family:monospace,monospace">       t->ratectr = (struct mgcp_ratectr_trunk){ 0 };</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what about this? I think there's APIs to reset the counters?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@377">Patch Set #32, Line 377:</a> <code style="font-family:monospace,monospace">           prctl(PR_SET_NAME, thrdname, NULL, NULL, NULL);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">POSIX threads have pthread_setname_np() for that.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@382">Patch Set #32, Line 382:</a> <code style="font-family:monospace,monospace">            log_enable_multithread();</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">iirc this needs to be called only once per program?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@383">Patch Set #32, Line 383:</a> <code style="font-family:monospace,monospace">                //msgb_talloc_ctx_init(OTC_GLOBAL,0); nooo is global!</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what about this?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@397">Patch Set #32, Line 397:</a> <code style="font-family:monospace,monospace">       osmo_stat_item_set(osmo_stat_item_group_get_item(t->stats.common, TRUNK_STAT_ENDPOINTS_TOTAL),</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is this safe?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No it's not, which is why the next patch that fixes the counters exists, since we don't want to fix libosmocore.</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/32/src/libosmo-mgcp/mgcp_threads.c@413">Patch Set #32, Line 413:</a> <code style="font-family:monospace,monospace">  //FIXME: shutdown</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">something to do with this?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/32/src/libosmo-mgcp/mgcp_threads.c@440">Patch Set #32, Line 440:</a> <code style="font-family:monospace,monospace">                 /* currently unused</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what about this?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: 32 </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: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 17 Nov 2021 20:34:30 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>