[asterisk-dev] [Code Review] Compare T.38 attributes in a case insensitive manner

[Sean Bright]

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

Ship it!


Looks good to me.  Minor note included - feel free to ignore.


/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/2298/#comment14753>

    Unrelated to the overall intent of the patch, but I would update these strncmp calls to something like this:
    
    strncmp(attrib, "foo", sizeof("foo") - 1)


- Sean


On Jan. 29, 2013, 9:13 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2298/
> -----------------------------------------------------------
> 
> (Updated Jan. 29, 2013, 9:13 a.m.)
> 
> 
> Review request for Asterisk Developers and wdoekes.
> 
> 
> Summary
> -------
> 
> RFC5347 section 2.5.2 states the following:
> ...
> The attribute "T38MaxBitRate" was once incorrectly registered with
> IANA as "T38maxBitRate" (lower-case "m"). In accordance with T.38
> examples and common implementation practice, the form "T38MaxBitRate"
> SHOULD be generated by implementations conforming to this package.
> In general, it is RECOMMENDED that implementations of this package
> accept lowercase, uppercase, and mixed upper/lowercase encodings of
> all the T.38 attributes.
> ...
> 
> Asterisk currently does not perform case insensitive matching on the T.38 attributes. This causes the T38MaxBitRate attribute to be negotiated at 2400 baud instead of 14400 (or whatever value you actually wanted).
> 
> Given that we use sscanf, there's two ways to go about solving this:
> 
> * Convert the received attribute to lower/upper case and compare that way (this is what this patch does)
> * Use sscanf to match both lower/upper case letters for every letter in the attribute, i.e., sscanf(a, "%*[tT]38%*[mM]%*[aA]%*[xX]%*[bB]%*[iI]%*[tT]%*[rR]%*[aA]%*[tT]%*[eE]:%30u", &x)
> 
> I went with the all lower option as it's a lot easier to read. The sscanf option is a bit more performant, as you don't have to convert the attribute to another case. (Note that as Walter suggested on ASTERISK-20897, we could write our own tolower method that did this a bit faster, but that feels like pre-mature optimization in this case).
> 
> If folks feel strongly about using a slightly faster approach, I'm open to that.
> 
> 
> This addresses bug ASTERISK-20897.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20897
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 380332 
> 
> Diff: https://reviewboard.asterisk.org/r/2298/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130129/98406a1f/attachment.htm>