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

[Joshua Colp]

Joshua Colp 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: Code-Review-1

(17 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 state somehow. Just dropping the ref doesn't do that.


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)?


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 means that any consumer has to stash the value away, use it, and then unref, instead of just calling this directly.


https://gerrit.asterisk.org/#/c/11462/1/include/asterisk/stasis_state.h@422 
PS1, Line 422: void stasis_state_publish_by_id_remove(struct stasis_state_manager *manager,
The naming of this feels odd, how about stasis_state_remove_publish_by_id?


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.


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@50 
PS1, Line 50: 	 * A container of eids. It's assumed that there is only a single publisher per
Single publisher per eid per topic.


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@67 
PS1, Line 67: 	/*! The manager's topic. All state topics are forward to this one */
We generally refer to these as "all" topics so using that name in here is probably good.


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?


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@210 
PS1, Line 210: 	state->num_subscribers = 0;
No need to do this since the allocation is zeroed out


https://gerrit.asterisk.org/#/c/11462/1/main/stasis_state.c@237 
PS1, Line 237: 				state->id ? state->id : "", stasis_topic_name(manager->topic));
This can be sad if state is NULL


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


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


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?


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.

It requires the stasis message payload of the last message.


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?


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?


https://gerrit.asterisk.org/#/c/11462/1/tests/test_stasis_state.c 
File tests/test_stasis_state.c:

https://gerrit.asterisk.org/#/c/11462/1/tests/test_stasis_state.c@322 
PS1, Line 322: 	stasis_state_publish(pub, msg);
test_stasis_state.c:322:2: error: ‘pub’ may be used uninitialized in this function [-Werror=maybe-uninitialized]



-- 
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-Comment-Date: Sun, 23 Jun 2019 17:33:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190623/b8f1dddf/attachment-0001.html>