[Asterisk-code-review] stasis_state: Add new stasis_state module (...asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11462 )

Change subject: stasis_state: Add new stasis_state module
......................................................................


Patch Set 1:

(11 comments)

https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h 
File include/asterisk/stasis_state.h:

https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@73 
PS1, Line 73:  * Subscribers are ao2 objects. Therefore there is no explicit cleanup required aside from
> For both publisher and subscriber this isn't quite true - in that it needs to be removed from the st […]
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.


https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@139 
PS1, Line 139: struct stasis_state_manager *stasis_state_manager_create(const char *topic_name);
> How is the termination of the state manager handled (if at all)?
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.


https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@353 
PS1, Line 353:  * \note Returned topic's reference count is incremented
> Is this necessary? As long as the ref to the publisher is kept, the topic should remain valid and it […]
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.


https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@425 
PS1, Line 425: /*! \brief Managed stasis state event interface */
> Should the observer API be in a separate header file? It feels out of place to have it in here.
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?


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c 
File main/stasis_state.c:

https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@182 
PS1, Line 182: 	ao2_ref(state_topic, +1);
> Why do you do this here instead of just calling ao2_bump when you set state->topic?
In order to consistently decrease the reference on state allocation error. I've updated the comment a bit to make it more obvious.


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@264 
PS1, Line 264: 	if (!id) {
> Should this use ast_strlen_zero instead?
Probably. Will change.


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@404 
PS1, Line 404: 	if (ao2_ref(state, 0) == 2 && AST_VECTOR_SIZE(&state->eids) == 0) {
> Explain the magic value of 2
Changed the "two" to a "2" to make the association in the comment more obvious.


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@506 
PS1, Line 506: 	ao2_ref(sub, -1);
> Why the unref here and below?
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.

It also balances/reverses the "subscribe" helper function (above this one) which first creates the state subscription, and then the topic subscription.


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@537 
PS1, Line 537: 	res = ao2_bump(stasis_message_data(sub->state->msg));
> I don't think the API call and documentation really matches what this does. […]
Updated docs on the prototype in the header


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@564 
PS1, Line 564: 	struct stasis_state_publisher *pub = ao2_alloc(sizeof(*pub), publisher_dtor);
> Does this need a lock?
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.


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@746 
PS1, Line 746: 	message_data = ao2_bump(stasis_message_data(state->msg));
> Should this really just pass in the msg?
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.

But can change if you think there is, or could be a use case for the message itself.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11462
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I7a4a06685a96e511da9f5bd23f9601642d7bd8e5
Gerrit-Change-Number: 11462
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 24 Jun 2019 22:41:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190624/f4a72d69/attachment-0001.html>