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

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

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


Patch Set 1:

(4 comments)

https://gerrit.asterisk.org/#/c/2709/1/include/asterisk/res_pjsip_outbound_publish.h
File include/asterisk/res_pjsip_outbound_publish.h:

PS1, Line 174: int ast_sip_publish_user_send(const char *user, const struct ast_sip_body *body);
             : 
             : /*!
             :  * \brief Remove the given user and stop outbound publishing for it
             :  *
             :  * \param user The user to remove
             :  */
             : void ast_sip_publish_user_remove(const char *user);
> This has to still take in a client. Each client has a set of users. Users s
What do you mean by "Users shouldn't be global"? For this function I could see how just passing in client (I'm guessing obtained from the datastore) alone could then be used to look up all associated states. Is that what you mean?


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

Line 200: static struct ao2_container *multi_publishes;
> Why did this need to be a global container instead of a container on the pu
There is only one publish [configuration] type per publish client. However now for a given publish type there may be 'n' publish clients (it was 1 to 1).

Publish clients are created when the state gets created (so still a 1 to 1 state/client relation). For a multi-user publish type the creation of the state is postponed until it is dynamically created in the client_user_send code. Because of this we need a mechanism to store the multi-user publish configuration type objects until such time, thus the global container.


Line 264: static AO2_GLOBAL_OBJ_STATIC(current_states);
> I don't think it makes sense anymore to have a global obj container that's 
It may not need to be anymore? However there are a few 'read' cases where it would be bad if the container was "swapped" out from underneath during one of these 'reads'.


Line 824: 	ao2_callback_data(multi_publishes, OBJ_NODATA,
> Doesn't this introduce a race condition where if another thread does this a
I thought the ao2_callback code would mutex lock the multi_publishes container? A second thread would be locked out until the first completed the callback/iteration.


-- 
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: 1
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