[asterisk-dev] [Code Review] 3960: res_pjsip_pubsub: Check supported headers for eventlist before allowing subscribe to resource list

[Mark Michelson]

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



/branches/13/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3960/#comment23686>

    For ease of reference, add \retval doxygen comments to this function.



/branches/13/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3960/#comment23685>

    The ast_copy_pj_str() here isn't needed. You can just do
    
    if (pj_stricmp2(&supported_header->values[i], "eventlist")) {
        return 1;
    }



/branches/13/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3960/#comment23687>

    With the way this is currently implemented, one of the off-nominal tests in the test plan will fail.
    
    Specifically, the test that will fail is List of Lists, category 2, ambiguity test 2. For that test, there are two resources with the same name. One resource is a list, and the other resource is a single item. The subscriber does not support resource lists, and the expectation is that the subscriber will get subscribed to the single resource.
    
    With the code as it currently is, that test will fail because the subscriber will get a 421 error.
    
    The if (!list) case in the top of this snippet should be run if a list does not have eventlist support. I usually don't like doing assignments in if statements, but this would suit the code pretty well here:
    
    if (!has_eventlist_support || !(list = retrieve_resource_list(resource, handler->event_name))) {
        ...
    }
    
    As far as returning 421 is concerned...I think that we should just drop the requirement of trying to return that response code. We can just return whatever handler->notifier->new_subscribe() returns.


- Mark Michelson


On Aug. 28, 2014, 9:59 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3960/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2014, 9:59 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Bugs: ASTERISK-23871
>     https://issues.asterisk.org/jira/browse/ASTERISK-23871
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> https://wiki.asterisk.org/wiki/display/AST/Resource+List+Subscription+Test+Plan
> 
> According to the off-nominal plan, if eventlist support is indicated in a subscribe's Supported header(s), subscriptions to a resource list should fail with a 421 extension not supported. This wasn't currently happening since the Supported header isn't actually checked by anything in the res_pjsip_pubsub code, so this patch adds a check for it.
> 
> 
> Diffs
> -----
> 
>   /branches/13/res/res_pjsip_pubsub.c 422272 
> 
> Diff: https://reviewboard.asterisk.org/r/3960/diff/
> 
> 
> Testing
> -------
> 
> Ran nominal RLS tests, ran in development off-nominal RLS tests, and ran Presence and MWI PJSIP subscription tests to confirm that they didn't break.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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