[asterisk-dev] [Code Review] 3628: RLS: Abstract PJSIP-specific components from the pubsub API

[Mark Michelson]

> On June 19, 2014, 7:26 p.m., Matt Jordan wrote:
> > /trunk/res/res_pjsip_pubsub.c, line 302
> > <https://reviewboard.asterisk.org/r/3628/diff/1/?file=59788#file59788line302>
> >
> >     Parent/child aside, I do enjoy the name of this union

Honestly, this was the hardest  part of the change. I really wish unions didn't require names. I actually consulted a thesaurus to try to come up with a single word that distinguishes between real and unreal things, and this is the one that made me cringe least.

During initial development, this union was called "pants" because I just needed some ridiculous placeholder that I could later easily find and replace.


> On June 19, 2014, 7:26 p.m., Matt Jordan wrote:
> > /trunk/res/res_pjsip_pubsub.c, lines 1359-1363
> > <https://reviewboard.asterisk.org/r/3628/diff/1/?file=59788#file59788line1359>
> >
> >     There's a lot of things later on in the code that seem to really want that resource to exist. Is there any risk here that request_uri->user (and thus resource) is empty or NULL? If this is empty/NULL, what happens when things request the resource later?

There's actually a test in the testsuite tests/channels/pjsip/subscriptions/mwi/missing_aor that has no username in the request URI.

What happens is that we end up creating an empty string and passing that off to the handler's new_subscribe() callback. It's up to the individual handler to determine how to interpret an empty resource. In the case of res_pjsip_exten_state, this will result in an error (a 404). However for MWI, this will result in subscribing to all AoRs configured for an endpoint. See https://issues.asterisk.org/jira/browse/ASTERISK-23072 for why the MWI handler behaves as it does.

One thing that is worth doing here though is ensuring that the request URI is a sip: or sips: URI before trying to cast it into a pjsip_sip_uri. Otherwise, we could have ourselves an issue.


> On June 19, 2014, 7:26 p.m., Matt Jordan wrote:
> > /trunk/res/res_pjsip_pubsub.c, lines 253-281
> > <https://reviewboard.asterisk.org/r/3628/diff/1/?file=59788#file59788line253>
> >
> >     I'm not against the 'real'/'virtual' nomenclature, but would 'parent'/'child' be better/clearer?

Hm, not necessarily. The problem is that a parent subscription may itself be a child of another subscription. Because of the hierarchy, the root of the tree is the only real subscription, while all descendants are virtual. So really, if you want to rename based on the structure, you'd maybe have root/child or root/descendant.

BTW, RFC 4662 uses the term "virtual subscription" which is why I use the term in the code. The "real" counterpart is an invention of my own.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3628/#review12224
-----------------------------------------------------------


On June 18, 2014, 9:12 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3628/
> -----------------------------------------------------------
> 
> (Updated June 18, 2014, 9:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23865
>     https://issues.asterisk.org/jira/browse/ASTERISK-23865
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This is step one towards implementing resource lists in Asterisk. Once resource list subscriptions are implemented, an ast_sip_subscription may not directly correspond to an underly pjsip_evsub structure. Currently, subscription handlers will gladly try to reach underneath the ast_sip_subscription to get at the PJSIP-specific structures. This set of changes seeks to abstract the pubsub API further so that subscription handlers do not have any interaction with PJSIP.
> 
> The changes outlined at https://wiki.asterisk.org/wiki/display/AST/PJSIP+Subscription+Abstraction+Plan provide a fairly accurate overview of what this review request entails. Some differences are:
> * I was not able to completely separate subscribers and notifiers into separate structures. Instead, they are now separate sub-structures within the ast_sip_subscription_handler struct. The reason for this is that PJSIP is not flexible enough to register separate entities for clients and servers.
> * The ast_sip_subscription_accept(), ast_sip_subscription_reject(), and ast_sip_subscription_terminate() calls ended up not being needed.
> * All functions on the page that have a body parameter as a const char * are implemented to use a pjsip_msg_body instead. This is a case where the underlying PJSIP structure does not need to be abstracted, and having a parsed structure rather than a text blob will benefit everyone.
> * One thing that is not on that page, but that you will find in this review, is that we now store the SUBSCRIBE request that starts a subscription on the subscription dialog. This allows subscription handlers to be able to retrieve the values of certain headers from the SUBSCRIBE if they wish. This is necessary for the exten_state handler currently since it wants the content of the User-Agent header.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_xpidf_body_generator.c 416652 
>   /trunk/res/res_pjsip_pubsub.exports.in 416652 
>   /trunk/res/res_pjsip_pubsub.c 416652 
>   /trunk/res/res_pjsip_pidf_body_generator.c 416652 
>   /trunk/res/res_pjsip_mwi.c 416652 
>   /trunk/res/res_pjsip_exten_state.c 416652 
>   /trunk/include/asterisk/res_pjsip_pubsub.h 416652 
> 
> Diff: https://reviewboard.asterisk.org/r/3628/diff/
> 
> 
> Testing
> -------
> 
> Since no new functionality has been added, I just ran the current gamut of testsuite tests (tests/channels/pjsip/subscriptions). They all pass.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140619/7c76599f/attachment-0001.html>