<p>Patch set 1:<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/c/asterisk/+/11462">View Change</a></p><p>17 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h">File include/asterisk/stasis_state.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.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@73">Patch Set #1, Line 73:</a> <code style="font-family:monospace,monospace"> * Subscribers are ao2 objects. Therefore there is no explicit cleanup required aside from</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">For both publisher and subscriber this isn't quite true - in that it needs to be removed from the state somehow. Just dropping the ref doesn't do that.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@139">Patch Set #1, Line 139:</a> <code style="font-family:monospace,monospace">struct stasis_state_manager *stasis_state_manager_create(const char *topic_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How is the termination of the state manager handled (if at all)?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@353">Patch Set #1, Line 353:</a> <code style="font-family:monospace,monospace"> * \note Returned topic's reference count is incremented</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this necessary? As long as the ref to the publisher is kept, the topic should remain valid and it means that any consumer has to stash the value away, use it, and then unref, instead of just calling this directly.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@422">Patch Set #1, Line 422:</a> <code style="font-family:monospace,monospace">void stasis_state_publish_by_id_remove(struct stasis_state_manager *manager,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The naming of this feels odd, how about stasis_state_remove_publish_by_id?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@425">Patch Set #1, Line 425:</a> <code style="font-family:monospace,monospace">/*! \brief Managed stasis state event interface */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should the observer API be in a separate header file? It feels out of place to have it in here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c">File main/stasis_state.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/11462/1/main/stasis_state.c@50">Patch Set #1, Line 50:</a> <code style="font-family:monospace,monospace">         * A container of eids. It's assumed that there is only a single publisher per</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Single publisher per eid per topic.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@67">Patch Set #1, Line 67:</a> <code style="font-family:monospace,monospace">   /*! The manager's topic. All state topics are forward to this one */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We generally refer to these as "all" topics so using that name in here is probably good.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@182">Patch Set #1, Line 182:</a> <code style="font-family:monospace,monospace">    ao2_ref(state_topic, +1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why do you do this here instead of just calling ao2_bump when you set state->topic?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@210">Patch Set #1, Line 210:</a> <code style="font-family:monospace,monospace">       state->num_subscribers = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need to do this since the allocation is zeroed out</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@237">Patch Set #1, Line 237:</a> <code style="font-family:monospace,monospace">                           state->id ? state->id : "", stasis_topic_name(manager->topic));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can be sad if state is NULL</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@264">Patch Set #1, Line 264:</a> <code style="font-family:monospace,monospace">    if (!id) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this use ast_strlen_zero instead?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@404">Patch Set #1, Line 404:</a> <code style="font-family:monospace,monospace">    if (ao2_ref(state, 0) == 2 && AST_VECTOR_SIZE(&state->eids) == 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Explain the magic value of 2</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@506">Patch Set #1, Line 506:</a> <code style="font-family:monospace,monospace">        ao2_ref(sub, -1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why the unref here and below?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@537">Patch Set #1, Line 537:</a> <code style="font-family:monospace,monospace">        res = ao2_bump(stasis_message_data(sub->state->msg));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think the API call and documentation really matches what this does.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It requires the stasis message payload of the last message.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@564">Patch Set #1, Line 564:</a> <code style="font-family:monospace,monospace">      struct stasis_state_publisher *pub = ao2_alloc(sizeof(*pub), publisher_dtor);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does this need a lock?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@746">Patch Set #1, Line 746:</a> <code style="font-family:monospace,monospace">   message_data = ao2_bump(stasis_message_data(state->msg));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this really just pass in the msg?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11462/1/tests/test_stasis_state.c">File tests/test_stasis_state.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/11462/1/tests/test_stasis_state.c@322">Patch Set #1, Line 322:</a> <code style="font-family:monospace,monospace"> stasis_state_publish(pub, msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">test_stasis_state.c:322:2: error: ‘pub’ may be used uninitialized in this function [-Werror=maybe-uninitialized]</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11462">change 11462</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/c/asterisk/+/11462"/><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-Change-Id: I7a4a06685a96e511da9f5bd23f9601642d7bd8e5 </div>
<div style="display:none"> Gerrit-Change-Number: 11462 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 23 Jun 2019 17:33:22 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>