[Asterisk-code-review] stasis.c: Added topic all container (asterisk[master])

[Corey Farrell]

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>