<p><a href="https://gerrit.asterisk.org/c/asterisk/+/11462">View Change</a></p><p>11 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">For both publisher and subscriber this isn't quite true - in that it needs to be removed from the st […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Dropping the ref should clean things up. At least once the subscriber's ref count reaches 0. Check the "subscriber_dtor "function. It calls "state_remove, which will remove the state under certain conditions.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How is the termination of the state manager handled (if at all)?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The state manager is an ao2_object and will be cleaned up once its ref count drops to 0. I'll add more text about 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@353">Patch Set #1, Line 353:</a> <code style="font-family:monospace,monospace"> * \note Returned topic's reference count is incremented</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this necessary? As long as the ref to the publisher is kept, the topic should remain valid and it […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not strictly necessary I guess. As you pointed out as long as a publisher and/or subscriber reference is held the topic will live. Consumers would just have to be careful not to retrieve the topic and then unref all the publishers or subscribers. I'll remove the ref bump.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should the observer API be in a separate header file? It feels out of place to have it in here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't feel that it is as it's part of, and directly related to the stasis state framework. If you feeling strongly though I can move it. Whatcha thinking on where to put it?</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@182">Patch Set #1, Line 182:</a> <code style="font-family:monospace,monospace">  ao2_ref(state_topic, +1);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why do you do this here instead of just calling ao2_bump when you set state->topic?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">In order to consistently decrease the reference on state allocation error. I've updated the comment a bit to make it more obvious.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this use ast_strlen_zero instead?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably. Will change.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Explain the magic value of 2</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Changed the "two" to a "2" to make the association in the comment more obvious.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why the unref here and below?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These are here as a convenience to the caller since in most every case of unsubscribing you'll want to remove the reference to the state subscriber.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It also balances/reverses the "subscribe" helper function (above this one) which first creates the state subscription, and then the topic subscription.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think the API call and documentation really matches what this does. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated docs on the prototype in the header</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does this need a lock?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">A lock on what? The manager->states object is locked, which it needs while it is finding and/or adding a state object to the manager.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this really just pass in the msg?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I debated which to pass. Settled on just the data since it seems like in every case that's what is needed. And it didn't seem like there was anything else on the message object that would be useful here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But can change if you think there is, or could be a use case for the message itself.</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-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 24 Jun 2019 22:41:44 +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: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>