<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10929">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c">File main/stasis.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.asterisk.org/#/c/10929/4/main/stasis.c@308">Patch Set #4, Line 308:</a> <code style="font-family:monospace,monospace">#define TOPIC_ALL_BUCKETS 997</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We might want an `#if defined(LOW_MEMORY)` block with a smaller number of buckets.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@376">Patch Set #4, Line 376:</a> <code style="font-family:monospace,monospace">   char buf[0];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You do not need the buffer in 'struct stasis_topic'.  name and detail can be pointed directly to the value associated with 'struct topic_proxy'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Once the two objects are created and the weak association created it is impossible for stasis_topic to outlive topic_proxy.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@384">Patch Set #4, Line 384:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">     /*! Variable length array of the subscribers */<br>       void *subscribers;<br><br>  /*! Topics forwarding into this topic */<br>      void *upstream_topics;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These pointers should not be on the weakproxy.  In *all* cases the 'struct statis_topic' will be destroyed before the associated 'struct topic_proxy'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It is safe for 'struct stasis_topic' to hold pointers to memory owned by 'struct topic_proxy', the other way around is not safe.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@459">Patch Set #4, Line 459:</a> <code style="font-family:monospace,monospace"> ao2_wrlock(topic_all);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">After locking the container you should probably check for an existing topic with `name`, abort with an error if found.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@461">Patch Set #4, Line 461:</a> <code style="font-family:monospace,monospace">        proxy = ao2_t_weakproxy_alloc(sizeof(*proxy), NULL, topic->name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The allocation needs to include space for name and detail to be stored in buf.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@463">Patch Set #4, Line 463:</a> <code style="font-family:monospace,monospace">          return 1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nit: could you `return -1` for errors?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Not a nit: you need to unlock topic_all in all error paths.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@466">Patch Set #4, Line 466:</a> <code style="font-family:monospace,monospace">       /* We have exclusive access to proxy and sorcery, no need for locking here. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/sorcery/topic/</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also please do the strcpy of name / detail into the proxy before this line.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@477">Patch Set #4, Line 477:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">   proxy->name = topic->name;<br>      proxy->detail = topic->detail;<br>  proxy->subscribers = &topic->subscribers;<br>   proxy->upstream_topics = &topic->upstream_topics;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please remove this.  proxy can't point to memory owned by topic.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@2232">Patch Set #4, Line 2232:</a> <code style="font-family:monospace,monospace">                       result = ast_strdup(topic->name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please use ast_cli_completion_add instead of returning the result.  This allows us to iterate the container once instead of repeatedly iterating.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10929">change 10929</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.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.asterisk.org/10929"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ie86d125d2966f93de74ee00f47ae6fbc8c081c5f </div>
<div style="display:none"> Gerrit-Change-Number: 10929 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 28 Jan 2019 00:52:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>