----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
