[Asterisk-code-review] stasis.c: Added topic all container (asterisk[master])
Corey Farrell
asteriskteam at digium.com
Mon Jan 28 08:30:05 CST 2019
Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/10929 )
Change subject: stasis.c: Added topic_all container
......................................................................
Patch Set 6: Code-Review-1
(4 comments)
https://gerrit.asterisk.org/#/c/10929/6/main/stasis.c
File main/stasis.c:
https://gerrit.asterisk.org/#/c/10929/6/main/stasis.c@476
PS6, Line 476: ast_log(LOG_ERROR, "Could not allocate the memory. name: %s\n", name);
A generic message is already printed via ao2_calloc. Honestly I'm not in favor of adding any of the LOG_ERROR messages in this function except for the "same topic already exists", that's the only non-system error that can occur here.
Also nit: I'd prefer the blank line in these error blocks be before the 'return -1'. This makes the fact that the blocks return stand out.
https://gerrit.asterisk.org/#/c/10929/6/main/stasis.c@2222
PS6, Line 2222: iter = ao2_iterator_init(topic_all, 0);
This CLI command should produce output sorted by name. The way I know to do this is:
* Create a temporary container with ao2_container_alloc_list (be sure to include a sort function).
* Use ao2_container_dup to copy items from topic_all to the temporary container.
* Iterate the temporary container. Using AO2_ITERATOR_UNLINK on ao2_iterator_init will avoid an extra ref/unref per item.
* Cleanup the iterator and temporary container.
https://gerrit.asterisk.org/#/c/10929/6/main/stasis.c@2252
PS6, Line 2252: if (!strncasecmp(word, topic->name, wordlen) && ++which > state) {
The which variable and state argument are not needed (this comparison is always true), they should be removed. When using ast_cli_completion_add the a->n (state) variable will always be 0.
https://gerrit.asterisk.org/#/c/10929/6/main/stasis.c@2253
PS6, Line 2253: ret = ast_cli_completion_add(ast_strdup(topic->name));
: ao2_ref(topic, -1);
: if (ret) {
: break;
: }
Currently this causes an extra call to ao2_ref(topic, -1) on the non-failure path. You can fix this and remove the 'ret' variable using the following code:
if (ast_cli_completion_add(ast_strdup(topic->name)) {
ao2_ref(topic, -1);
break;
}
--
To view, visit https://gerrit.asterisk.org/10929
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie86d125d2966f93de74ee00f47ae6fbc8c081c5f
Gerrit-Change-Number: 10929
Gerrit-PatchSet: 6
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Mon, 28 Jan 2019 14:30:05 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190128/5363a0b8/attachment.html>
More information about the asterisk-code-review
mailing list