[Asterisk-code-review] stasis.c: Added topic_all container (...asterisk[16])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11013 )

Change subject: stasis.c: Added topic_all container
......................................................................


Patch Set 5: Code-Review-1

(10 comments)

https://gerrit.asterisk.org/#/c/11013/5/include/asterisk/stasis.h 
File include/asterisk/stasis.h:

https://gerrit.asterisk.org/#/c/11013/5/include/asterisk/stasis.h@518 
PS5, Line 518: struct stasis_topic *stasis_topic_create(const char *name);
I think topic names ultimately should be unique and descriptive themselves. To that end I'm going to be auditing the codebase and improving this for topics and subscriptions. I think the cleanest option is probably to have this code do a check for a topic of the same name when creating, and if one exists then to append a unique part to the end of the passed in name. Example:

A topic named "pjsip" is created.
Another topic named "pjsip" is created but the code appends some unique data turning it into "pjsip-349d93".

This guarantees the topic names are unique, and as the topic names are not actually used by the API itself - whether the name passed in is used or not doesn't actually matter.


https://gerrit.asterisk.org/#/c/11013/5/include/asterisk/stasis.h@521 
PS5, Line 521:  * \brief Create a new topic with given detail.
This now creates one with uniqueid and detail


https://gerrit.asterisk.org/#/c/11013/5/include/asterisk/stasis.h@522 
PS5, Line 522:  * \param uniqueid Uniqueid of the new topic.
             :  * \param name Name of the new topic.
             :  * \param detail Detail of the new topic.
To me this seems as though things are getting overloaded with different fields.

What's the difference between the uniqueid, name, and detail. What should they all provide? What should a topic creator place in each? What's the difference between topic and uniqueid? What should go into detail?

A topic name is supposed to adequately describe its purpose, so I'm not sure what detail adds.


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c 
File main/stasis.c:

https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@434 
PS5, Line 434: 	ast_log(LOG_DEBUG, "Destroying topic. uniqueid: %s, name: %s, detail: %s\n",
This should use ast_debug with a sufficiently high log level.


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@487 
PS5, Line 487: 	if (!topic || !name || !detail) {
What if they're empty?


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@495 
PS5, Line 495: 		ast_log(LOG_ERROR, "The same topic is already exist. name: %s\n", name);
This should use ast_debug with a sufficiently high log level.


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@557 
PS5, Line 557: 	if (!uniqueid || !name || !detail) {
What if these are empty?


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@560 
PS5, Line 560: 	ast_log(LOG_DEBUG, "Creating topic. uniqueid: %s, name: %s, detail: %s\n",
This should use ast_debug with a sufficiently high log level.


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@565 
PS5, Line 565: 		ast_log(LOG_DEBUG, "Topic is already exist. uniqueid:%s, name: %s, detail: %s\n",
This should use ast_debug with a sufficiently high log level.


https://gerrit.asterisk.org/#/c/11013/5/main/stasis.c@1172 
PS5, Line 1172: 	tmp_subscription = AST_VECTOR_GET_CMP(&topic->subscribers, sub, TOPIC_SUBSCRIPTION_CMP);
              : 	if (!tmp_subscription) {
              : 		AST_VECTOR_APPEND(&topic->subscribers, sub);
              : 	}
In what case will the subscription be added twice? Have you encountered a scenario?

This seems like a separate change/fix.



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ie86d125d2966f93de74ee00f47ae6fbc8c081c5f
Gerrit-Change-Number: 11013
Gerrit-PatchSet: 5
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 11:48:53 +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/20190307/c14481b5/attachment.html>