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

[sungtae kim]

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

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


Patch Set 6:

(9 comments)

@corey, Thanks. :) I've fixed it all.

https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c
File main/stasis.c:

https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@308
PS4, Line 308: #if defined(LOW_MEMORY)
> We might want an `#if defined(LOW_MEMORY)` block with a smaller number of buckets.
Yes, I've fixed it. :)


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@376
PS4, Line 376: 
> You do not need the buffer in 'struct stasis_topic'. […]
Thanks, I've fixed it.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@384
PS4, Line 384: struct ao2_container *topic_all;
             : 
             : struct topic_proxy {
             : 	AO2_WEAKPROXY();
             : 
> These pointers should not be on the weakproxy. […]
Thanks, I've fixed it.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@459
PS4, Line 459: 	if (!topic || !name || !detail) {
> After locking the container you should probably check for an existing topic with `name`, abort with  […]
Sure, fixed it. :)


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@461
PS4, Line 461: 	}
> The allocation needs to include space for name and detail to be stored in buf.
Yes, fixed it.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@463
PS4, Line 463: 	ao2_wrlock(topic_all);
> Nit: could you `return -1` for errors? […]
No problem.

Return -1 when it's an error. And added unlock as well.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@466
PS4, Line 466: 	if (topic_tmp) {
> s/sorcery/topic/ […]
Sure, fixed it.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@477
PS4, Line 477: 
             : 		ao2_unlock(topic_all);
             : 		return -1;
             : 	}
> Please remove this.  proxy can't point to memory owned by topic.
Sure, fixed it. :)


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@2232
PS4, Line 2232: #undef FMT_HEADERS
> Please use ast_cli_completion_add instead of returning the result. […]
Yes, fixed it! :)



-- 
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 13:43:44 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190128/52ecca19/attachment.html>