[asterisk-dev] [Code Review] 2445: Pimp my SIP: Media Negotiations

[Mark Michelson]

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



/team/group/pimp_my_sip/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2445/#comment16108>

    I'd add the word SIP in here just so it's clear that this is only applicable to SIP channels.



/team/group/pimp_my_sip/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2445/#comment16109>

    Print a warning if someone gives a format whose name we don't recognize.



/team/group/pimp_my_sip/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2445/#comment16110>

    The size arguments for these strncat calls are not useful. strncat is meant to take the maximum amount of space it can add onto the buffer, not the size of the string being added.
    
    In this case, you're already adjusting the remaining len based on the size of the name plus a comma. Since you've already ensured the buffer has the space for the name and comma, you can get away with using strcat instead of strncat. Just put a comment above the strcat calls indicating they're being used safely. Some people have quick trigger fingers and will "correct" their usage without understanding that they're being used safely.



/team/group/pimp_my_sip/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2445/#comment16111>

    Hmm. For the purposes of synchronization, we may need to look into pushing this into the session's serializer as a synchronous task. The reason is that you're messing with session capabilities and it may be possible that another thread is either could be trying to read or write session capabilities too.
    
    The reason I've unchecked the "Open an issue" box is that this function really should only be used before we've even placed an outbound call, so when used "correctly" there shouldn't be any other thread that could be messing with the session capabilities. However, if someone were to try to make use of this function mid-call, they might end up with some oddball data set for the session capabilities.


- Mark Michelson


On April 25, 2013, 4:20 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2445/
> -----------------------------------------------------------
> 
> (Updated April 25, 2013, 4:20 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21186
>     https://issues.asterisk.org/jira/browse/ASTERISK-21186
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Added a dialplan function MEDIA_OFFER that accepts a codec type (example: 'audio') and allows overriding, or re-ordering, of an endpoints codecs prior to dialing (e.g. using a pre-dial handler).  This adds functionality for outbound requests only.
> 
> Example: Set(MEDIA_OFFER(audio)=ulaw,g722) ; sets the outgoing codecs to be ulaw,g722
> 
> Note that using this function and setting new media offers completely overrides what is specified on the endpoint.  Currently it is allowed to even list a codec that was not previously specified on the endpoint.
> 
> The code allows for un/registering of media offer types that can be associated with the function itself.  This allows for future expansion of other types, for example T.38.  Types 'audio' and 'video' are currently supported.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/channels/chan_gulp.c 386530 
>   /team/group/pimp_my_sip/include/asterisk/res_sip_session.h 386530 
>   /team/group/pimp_my_sip/res/res_sip_sdp_rtp.c 386530 
>   /team/group/pimp_my_sip/res/res_sip_session.c 386530 
>   /team/group/pimp_my_sip/res/res_sip_session.exports.in 386530 
> 
> Diff: https://reviewboard.asterisk.org/r/2445/diff/
> 
> 
> Testing
> -------
> 
> Ran through several scenarios setting new MEDIA_OFFER(s).  Tested re-ordering of already specified codecs on an endpoint, tested setting only a single codec (both specified and not on endpoint).  Tested reading back out the newly set codecs in the dialplan.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130425/b836cf7b/attachment-0001.htm>