[asterisk-dev] [Code Review] 3150: Decouple Subscription handling and NOTIFY/PUBLISH body generation

[Joshua Colp]

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

Ship it!


Uhhhh, I've looked over this twice and come up with nothing. Just some redness, but there are eye drops for that. I'd get another look though.


/trunk/res/res_pjsip_pidf_nonstandard_body_supplement.c
<https://reviewboard.asterisk.org/r/3150/#comment20162>

    I'm not a huge fan of calling this nonstandard, purely because it's so generic and could apply to other stuff. Might as well just go ahead and call it eyebeam since that is where it originated.


- Joshua Colp


On Jan. 23, 2014, 10:52 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3150/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2014, 10:52 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When the PJSIP pubsub framework was created, subscription handlers were required to state what event they handled along with what body types they knew how to generate. While this serves well when implementing a base RFC, it has problems when trying to extend the body to support non-standard or proprietary body elements. The code also was NOTIFY-specific, meaning that when the time comes that we start writing code to send out PUBLISH requests with MWI or presence bodies, we would likely find ourselves duplicating code that had previously been written.
> 
> This changeset introduces the concept of body generators and body supplements. A body generator is responsible for allocating a native structure for a given body type, providing the primary body content, converting the native structure to a string, and deallocating resources. A body supplement takes the primary body content (the native structure, not a string) generated by the body generator and adds nonstandard elements to the body. With these elements living in their own module, it becomes easy to extend our support for body types and to re-use resources when sending a PUBLISH request.
> 
> Body generators and body supplements register themselves with the pubsub core, similar to how subscription and publish handlers had done. Now, subscription handlers do not need to know what type of body content they generate, but they still need to inform the pubsub core about what the default body type for a given event package is. The pubsub core keeps track of what body generators and body supplements have been registered. When a SUBSCRIBE arrives, the pubsub core will check that there is a subscription handler for the event in the SUBSCRIBE, then it will check that there is a body generator that can provide the content specified in the Accept header(s).
> 
> Because of the nature of body generators and supplements, it means res_pjsip_exten_state and res_pjsip_mwi have been completely gutted. They no longer worry about body types, instead calling ast_sip_pubsub_generate_body_content() when they need to generate a NOTIFY body.
> 
> Some potentially dodgy elements in play here:
> * The use of void pointers for body allocation and generation means that reading the source will be necessary in order to figure out what type of data is being passed to a body supplement that you add. This information could probably stand to be documented someplace.
> * There is no module reference counting happening here. If there is a live subscription and the body generator for that subscription is unloaded, some terrible things are likely to happen.
> 
> NOTE:
> There are red blobs on the initial diff that is being posted here. I have already fixed these in the branch that I have been working in, so there is no need to point them out during review.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_xpidf_body_generator.c PRE-CREATION 
>   /trunk/res/res_pjsip_pubsub.exports.in 406303 
>   /trunk/res/res_pjsip_pubsub.c 406303 
>   /trunk/res/res_pjsip_pidf_nonstandard_body_supplement.c PRE-CREATION 
>   /trunk/res/res_pjsip_pidf_body_generator.c PRE-CREATION 
>   /trunk/res/res_pjsip_pidf.c 406303 
>   /trunk/res/res_pjsip_mwi_body_generator.c PRE-CREATION 
>   /trunk/res/res_pjsip_mwi.c 406303 
>   /trunk/res/res_pjsip_exten_state.c 406303 
>   /trunk/res/res_pjsip/presence_xml.c PRE-CREATION 
>   /trunk/include/asterisk/res_pjsip_pubsub.h 406303 
>   /trunk/include/asterisk/res_pjsip_presence_xml.h PRE-CREATION 
>   /trunk/include/asterisk/res_pjsip_exten_state.h 406303 
>   /trunk/include/asterisk/res_pjsip_body_generator_types.h PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3150/diff/
> 
> 
> Testing
> -------
> 
> In addition to manual testing, I have also posted https://reviewboard.asterisk.org/r/3151 that contains new tests for the testsuite.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

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