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

[Sean Bright]

> On Jan. 29, 2013, 10:35 a.m., Sean Bright wrote:
> > /branches/1.8/channels/chan_sip.c, line 10238
> > <https://reviewboard.asterisk.org/r/2298/diff/1/?file=33124#file33124line10238>
> >
> >     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)

Just noticed this was against 1.8.  If you do update these I would only do it in trunk.


- Sean


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


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/b2f3897d/attachment.htm>