[asterisk-dev] [Code Review] 3780: res_pjsip_outbound_publish / res_pjsip_publish_asterisk: Add outbound PUBLISH support with 'asterisk' event type.

[Mark Michelson]

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



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23014>

    Should note that this is expected to be a SIP URI, not a bare IP address.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23015>

    This seems like an odd default behavior. I would suspect that by default we would actually magic up a From URI of our own based instead of telling the server that the message is coming from itself.
    
    Doing this when no to_uri is provided makes good sense though.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23041>

    Passing a NULL cb_fn here is likely not what you intended. This will cause ao2_callback to match on every object in the container. This means that the name parameter will be ignored. Since OBJ_MULTIPLE was not supplied, that means that the first datastore in the container will always be removed.
    
    What will work better here is:
    
    ao2_find(client->datastores, name, OBJ_SEARCH_KEY | OBJ_UNLINK | OBJ_NODATA);
    
    Note that the cast of name is not necessary with ao2_find since it takes a const parameter already.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23040>

    The control flow being used here bothers me. The types of errors that lead to this label are errors that are likely to persist across multiple attempts to send the same message. Since this method does not actually remove the first message from the queue, it means that each call to sip_publish_client_service_queue() is likely to encounter the same error repeatedly, resulting in the queue getting "stuck" forever.
    
    I think what needs to be done is to remove the front message from the queue and try to send it. If an error is encountered, then go back to trying to service the queue again like you have here. The difference is that now when you service the queue, you'll be pulling the next message from the queue instead of trying to send the same one again.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23049>

    Since you are using the auto_refresh capability in PJSIP's publishc API, this can be hazardous. If PJSIP sends a publication refresh packet, then you'll get called back here. In such a situation, you're essentially getting an extra PUBLISH response that you're not expecting. This will result in popping the head of the queue off when the head of the queue has not actually been sent yet. You'll leak the memory and end up not ever sending the message that was at the head of the queue.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23030>

    None of this is necessary. pjsip_publishc_init() parses URIs and will return PJSIP_EINVALIDURI if any URIs are invalid. You won't have the granularity that you currently have here, but you'll save on the repeated parsing of URIs.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23023>

    Currently, this declaration of tmp is shadowing the outer declaration of tmp. However, if you get rid of all the URI parsing in this function, then you'll get rid of the outer tmp, and so the shadowing won't be a problem any longer.
    
    However, in addition, you really don't need tmp here. Since publish->outbound_proxy is already a NULL-terminated string, you don't need to perform the pj_strdup2_with_null(). You can just pass publish->outbound_proxy and strlen(publish->outbound_proxy) as the parameters to pjsip_parse_hdr.



/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23018>

    So, seeing this handler in action leads me to realize that this is pretty much duplicating sorcery extended object fields. Any reason we couldn't use those for event-specific configuration instead of having the "configure_" stuff?



/trunk/res/res_pjsip_publish_asterisk.c
<https://reviewboard.asterisk.org/r/3780/#comment23045>

    Is there some sort of stasis cache removal you could perform here?



/trunk/res/res_pjsip_publish_asterisk.c
<https://reviewboard.asterisk.org/r/3780/#comment23044>

    My recollection here is that you don't need to check for a NULL return from ast_strdupa(). My man entry for alloca() makes no mention of being capable of returning NULL at least.



/trunk/res/res_pjsip_publish_asterisk.c
<https://reviewboard.asterisk.org/r/3780/#comment23043>

    I think it would be fine to use pj_strcmp2 instead of pj_stricmp2 here.



/trunk/res/res_pjsip_pubsub.c
<https://reviewboard.asterisk.org/r/3780/#comment23046>

    Comment explaining the "+ 6" please.


- Mark Michelson


On July 14, 2014, 3:25 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3780/
> -----------------------------------------------------------
> 
> (Updated July 14, 2014, 3:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This adds two PJSIP modules which add outbound PUBLISH support and an 'asterisk' event type.
> 
> The res_pjsip_outbound_publish module is a common module which provides basic logic for setting up outbound PUBLISH clients, handling authentication requests, handling configuration, and lifetime. Extra modules implement specific event types which are registered with res_pjsip_outbound_publish. Since it takes care of configuration when an outbound PUBLISH is configured extra configuration can be passed to the event type implementation to further configure itself.
> 
> The res_pjsip_publish_asterisk module implements inbound and outbound support for an 'asterisk' event type. This event type conveys device and mailbox state between Asterisk instances using a JSON content body. As internal device or mailbox state changes the module sends a PUBLISH message to other configured instances. When a PUBLISH is received the contents are examined and a device or mailbox state change queued up within Asterisk. To restrict what is sent and received filtering is available using regular expressions which can reduce SIP traffic.
> 
> A wiki page is available at https://wiki.asterisk.org/wiki/display/~jcolp/Exchanging+Device+and+Mailbox+State+Using+PJSIP which has some configuration details with some examples. This should also be reviewed.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_pubsub.exports.in 418582 
>   /trunk/res/res_pjsip_pubsub.c 418582 
>   /trunk/res/res_pjsip_publish_asterisk.c PRE-CREATION 
>   /trunk/res/res_pjsip_outbound_publish.exports.in PRE-CREATION 
>   /trunk/res/res_pjsip_outbound_publish.c PRE-CREATION 
>   /trunk/include/asterisk/res_pjsip_pubsub.h 418582 
>   /trunk/include/asterisk/res_pjsip_outbound_publish.h PRE-CREATION 
>   /trunk/configs/pjsip.conf.sample 418582 
> 
> Diff: https://reviewboard.asterisk.org/r/3780/diff/
> 
> 
> Testing
> -------
> 
> Set up two Asterisk instances, configured both sides to publish to eachother, made calls and manipulated voicemail. Watched PUBLISH messages go between them and state change.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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