[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 4: Code-Review-1

(9 comments)

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: #define TOPIC_ALL_BUCKETS 997
We might want an `#if defined(LOW_MEMORY)` block with a smaller number of buckets.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@376
PS4, Line 376: 	char buf[0];
You do not need the buffer in 'struct stasis_topic'.  name and detail can be pointed directly to the value associated with 'struct topic_proxy'.

Once the two objects are created and the weak association created it is impossible for stasis_topic to outlive topic_proxy.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@384
PS4, Line 384: 	/*! Variable length array of the subscribers */
             : 	void *subscribers;
             : 
             : 	/*! Topics forwarding into this topic */
             : 	void *upstream_topics;
These pointers should not be on the weakproxy.  In *all* cases the 'struct statis_topic' will be destroyed before the associated 'struct topic_proxy'.

It is safe for 'struct stasis_topic' to hold pointers to memory owned by 'struct topic_proxy', the other way around is not safe.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@459
PS4, Line 459: 	ao2_wrlock(topic_all);
After locking the container you should probably check for an existing topic with `name`, abort with an error if found.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@461
PS4, Line 461: 	proxy = ao2_t_weakproxy_alloc(sizeof(*proxy), NULL, topic->name);
The allocation needs to include space for name and detail to be stored in buf.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@463
PS4, Line 463: 		return 1;
Nit: could you `return -1` for errors?

Not a nit: you need to unlock topic_all in all error paths.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@466
PS4, Line 466: 	/* We have exclusive access to proxy and sorcery, no need for locking here. */
s/sorcery/topic/

Also please do the strcpy of name / detail into the proxy before this line.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@477
PS4, Line 477: 	proxy->name = topic->name;
             : 	proxy->detail = topic->detail;
             : 	proxy->subscribers = &topic->subscribers;
             : 	proxy->upstream_topics = &topic->upstream_topics;
Please remove this.  proxy can't point to memory owned by topic.


https://gerrit.asterisk.org/#/c/10929/4/main/stasis.c@2232
PS4, Line 2232: 			result = ast_strdup(topic->name);
Please use ast_cli_completion_add instead of returning the result.  This allows us to iterate the container once instead of repeatedly iterating.



-- 
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: 4
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 00:52:08 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190127/92412892/attachment.html>