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

[sungtae kim]

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

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


Patch Set 8:

(9 comments)

> Patch Set 7: Code-Review-1
> 
> (9 comments)

Hi Joshua,

Thank you for your kind reviewing. I've fixed as your comment with replies. :)

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

https://gerrit.asterisk.org/#/c/11013/7/include/asterisk/stasis.h@524 
PS7, Line 524:  * \param uniqueid Uniqueid of the new topic.
> This isn't passed in.
Thanks, fixed it. :)


https://gerrit.asterisk.org/#/c/11013/7/include/asterisk/stasis.h@526 
PS7, Line 526:  * \param detail Detail of the new topic.
> What does detail mean exactly? What should be placed in here?
I expect some detail description of the topic.

i.e. Queue main topic for subscribing every queue event

Added comment for that.


https://gerrit.asterisk.org/#/c/11013/7/include/asterisk/stasis.h@546 
PS7, Line 546: struct stasis_topic *stasis_topic_get(const char *uniqueid);
> This passes in name now, so it should be named appropriately.
Oops! Fixed it.


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

https://gerrit.asterisk.org/#/c/11013/7/main/stasis.c@435 
PS7, Line 435: 	ast_log(LOG_DEBUG, "Destroying topic. name: %s, detail: %s\n",
> This should use ast_debug with a sufficiently high debug level
Np, fixed it.


https://gerrit.asterisk.org/#/c/11013/7/main/stasis.c@491 
PS7, Line 491: 	if (!topic || !name || !detail) {
> What if name is empty?
Agree, fixed to check the length.


https://gerrit.asterisk.org/#/c/11013/7/main/stasis.c@557 
PS7, Line 557: 	if (!name || !detail) {
> What if name is empty?
Agree, fixed to check the length.


https://gerrit.asterisk.org/#/c/11013/7/main/stasis.c@560 
PS7, Line 560: 	ast_log(LOG_DEBUG, "Creating topic. name: %s, detail: %s\n", name, detail);
> This should use ast_debug with a sufficiently high debug level
Np, fixed it.


https://gerrit.asterisk.org/#/c/11013/7/main/stasis.c@564 
PS7, Line 564: 		ast_log(LOG_DEBUG, "Topic is already exist. name: %s, detail: %s\n",
> This should use ast_debug with a sufficiently high debug level
Np, fixed it.


https://gerrit.asterisk.org/#/c/11013/7/main/stasis.c@1184 
PS7, Line 1184: tmp_subscription = AST_VECTOR_GET_CMP(&topic->subscribers, sub, TOPIC_SUBSCRIPTION_CMP);
              : 	if (!tmp_subscription) {
              : 		AST_VECTOR_APPEND(&topic->subscribers, sub);
              : 	}
> This seems like a separate bug fix, and did you encounter a case where this happened?
Yes, I agree with it.

No, haven't seen that case yet, I just couldn't pass it.
Is it better to create new ticket only for this?



-- 
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: 8
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, 21 Mar 2019 21:28:07 +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/20190321/92ae0731/attachment.html>