[asterisk-dev] [Code Review] CEL cel_odbc backend fails to insert eventtype into integer or other numeric column

[Tilghman Lesher]

> On 2011-04-03 22:39:40, Tilghman Lesher wrote:
> > /trunk/cel/cel_odbc.c, line 548
> > <https://reviewboard.asterisk.org/r/1160/diff/2/?file=16055#file16055line548>
> >
> >     This value should not have decremented to 15, as the format string still has a maximum result length of 17.
> 
> kkm wrote:
>     Yuck. 17. I removed the spaces just inside {} but did not recount.

I'm sure the spaces don't make much of a difference, functionally, but they were there for readability.


> On 2011-04-03 22:39:40, Tilghman Lesher wrote:
> > /trunk/cel/cel_odbc.c, line 562
> > <https://reviewboard.asterisk.org/r/1160/diff/2/?file=16055#file16055line562>
> >
> >     Why is this necessary?
> 
> kkm wrote:
>     Leap second. At least, linux and bsd systems have this in /usr/include/time.h:
>     
>     struct tm
>     {
>       int tm_sec;                   /* Seconds.     [0-60] (1 leap second) */

Right, but your database time code should handle these (exceptionally rare) cases correctly.  The danger of not handling them is that this introduces a possible billing error into the system.  Yes, it's only a second, but that could introduce a difference between billing an additional increment or not.

If your database doesn't handle those cases correctly, then you need to file a bug, and include (possibly conditionally compiled) code to workaround that bug in the database in the meantime.

I'm sure we have that bug in Asterisk, too, so it may be worth allocating an issue report to look at all of these cases and explicitly mention it in the code guidelines.


> On 2011-04-03 22:39:40, Tilghman Lesher wrote:
> > /trunk/cel/cel_odbc.c, line 574
> > <https://reviewboard.asterisk.org/r/1160/diff/2/?file=16055#file16055line574>
> >
> >     Why is this changing from 15 to 16?  Maximum length of the format string is still 15.
> 
> kkm wrote:
>     Yes, 15 here, with spaces removed.
>     
>     Just wondering, what is the point of using these macros here? ast_str_append() extends the string automatically anyway. Is that so we can check for memory allocation errors?

No, it checks for memory allocation errors, anyway.  The point of preallocating space is that it's slightly faster, because the format code (somewhat expensive operation) doesn't have to run twice.  (See the man page for vsnprintf(3), the function behind the abstraction layer.)  The more format specifiers within the string, the more expensive the operation.  Note that this code does not necessarily cause memory allocation, either, because the previous uses of the buffer may not have used all of the space allocated for it (it uses worst-case preallocation).  All the ast_str_make_space() function will do in that case is ensure that at least that number of bytes are available in the buffer.


- Tilghman


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


On 2011-04-04 13:13:25, kkm wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1160/
> -----------------------------------------------------------
> 
> (Updated 2011-04-04 13:13:25)
> 
> 
> Review request for Asterisk Developers and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> If eventtype is numeric in the database, it gets set to NULL, not to one of the numeric values as documented in cel_odbc.conf.sample.
> 
> 
> This addresses bug 18964.
>     https://issues.asterisk.org/view.php?id=18964
> 
> 
> Diffs
> -----
> 
>   /trunk/cel/cel_odbc.c 312554 
> 
> Diff: https://reviewboard.asterisk.org/r/1160/diff
> 
> 
> Testing
> -------
> 
> After preproduction testing, the change deployed to large scale production since March, 21.
> 
> 
> Thanks,
> 
> kkm
> 
>

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