[Asterisk-code-review] res pjsip outbound publish: Add multi-user support per confi... (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: res_pjsip_outbound_publish: Add multi-user support per configuration
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/2709/3/res/res_pjsip_outbound_publish.c
File res/res_pjsip_outbound_publish.c:

PS3, Line 153: 	char *user;
You can optimize this by allocating both the user and transport name as part of the allocation for the structure itself. It also saves you from having to free it when destroying it.


PS3, Line 155: 	struct ast_sip_outbound_publish_client *owner;
Does this create a circular reference at all?


PS3, Line 195:  * Mutli-user configurations make it so publishers can be dynamically added and
             :  * removed. Publishers should not be added or removed during a [re]load since
             :  * it could cause the current_clients container to be out of sync. Thus the
             :  * reason for this lock.
Ew, but I understand why.


Line 920: 	publisher->user = ast_strdup(user ? user : "");
S_OR here


Line 1020: 		handler->stop_publishing(client);
I don't think this is the right place to call stop_publishing. It will end up getting called for every publisher when it should be called only once for the client when the client overall is stopped.


PS3, Line 1057: 	ao2_callback(state->client->publishers, OBJ_NODATA | OBJ_UNLINK, cancel_and_unpublish, NULL);
              : 	ao2_cleanup(state->client->publishers);
              : 
              : 	ao2_cleanup(state->client);
You may want to document the current relationship between objects and the lifecycle of each and what causes what to happen.


-- 
To view, visit https://gerrit.asterisk.org/2709
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88dde024cc83c916424645d4f5bb84a0fa936cc
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes