[asterisk-dev] [Code Review] GSOC 2010 - Data API providers

Wed Jul 7 14:43:03 CDT 2010

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_dahdi.c, lines 17530-17532
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11357#file11357line17530>
> >
> >     You were correct that this was copied correctly; however, I just verified that the code was incorrect.  Fixed in trunk.

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_dahdi.c, lines 17595-17596
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11357#file11357line17595>
> >
> >     You could probably just do this in the initializer for the structure and save a few bytes, i.e.
> >     
> >     struct dahdi_versioninfo vi = { .version = "Unknown", .echo_canceller = "Unknown", };

Yes, fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, line 14208
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14208>
> >
> >     AST_DATA_SECONDS

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, line 14210
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14210>
> >
> >     Another scheduler entry

Removed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, line 14207
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14207>
> >
> >     This is a scheduler pointer entry and is thus useless outside of Asterisk.

Removed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, lines 14211-14214
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14211>
> >
> >     All of these should be AST_DATA_MILLISECONDS

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, line 14216
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14216>
> >
> >     AST_DATA_BOOLEAN

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27596
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27596>
> >
> >     The purpose of this boolean is merely for updating the realtime backend, so it probably doesn't need to be exposed.

Removed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, line 14283
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14283>
> >
> >     Should be an enumeration (plaintext, md5, rsa)

Fixed,
  I have used booleans, because multiple methods are allowed at once.
<authmethods>
     <rsa>true</rsa>
     <plaintext>true</plaintext>
     <md5>false</md5>
</authmethods>

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_iax2.c, line 14280
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11358#file11358line14280>
> >
> >     Probably should be an enumeration (omit, billing, documentation)

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27607
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27607>
> >
> >     Scheduler entry.

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, lines 27608-27610
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27608>
> >
> >     AST_DATA_SECONDS

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27611
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27611>
> >
> >     Another scheduler entry

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, lines 27612-27616
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27612>
> >
> >     These are all AST_DATA_MILLISECONDS

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/main/channel.c, line 175
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11365#file11365line175>
> >
> >     Enumeration

Fixed

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/main/channel.c, line 146
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11365#file11365line146>
> >
> >     This field is actually completely unused.

Removed.

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/main/channel.c, line 188
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11365#file11365line188>
> >
> >     Enumeration

Fixed.

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27605
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27605>
> >
> >     This is a set of booleans, that should probably be broken out.  See channels/sip/include/sip.h, in sip_options, for the translations.

Done. Thanks for the tip.

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27604
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27604>
> >
> >     This is actually an encoded integer, composed of the upper 16 bits, which are the new mailbox messages and the lower 16 bits, which are the old mailbox messages.  Given that that information is available elsewhere, I would suggest dropping this field from the export.

Removed.

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27597
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27597>
> >
> >     Enumeration

Done

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27591
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27591>
> >
> >     Possibly an enumeration here.

I don't have an integer value to create an enumeration, it is just the name of the rtp engine.

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/channels/chan_sip.c, line 27598
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11359#file11359line27598>
> >
> >     Enumeration

Done.

> On 2010-07-06 17:46:43, Tilghman Lesher wrote:
> > /trunk/main/channel.c, line 145
> > <https://reviewboard.asterisk.org/r/757/diff/3/?file=11365#file11365line145>
> >
> >     Could go for enumeration here.

Done.

- Eliel

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

On 2010-07-03 22:17:33, Eliel Sardañons wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/757/
> -----------------------------------------------------------
> 
> (Updated 2010-07-03 22:17:33)
> 
> 
> Review request for Asterisk Developers and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> Data API providers already implemented for the summer of code 2010 project
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_meetme.c 273879 
>   /trunk/apps/app_queue.c 273879 
>   /trunk/apps/app_voicemail.c 273879 
>   /trunk/channels/chan_agent.c 273879 
>   /trunk/channels/chan_dahdi.c 273879 
>   /trunk/channels/chan_iax2.c 273879 
>   /trunk/channels/chan_sip.c 273879 
>   /trunk/include/asterisk/cdr.h 273879 
>   /trunk/include/asterisk/channel.h 273879 
>   /trunk/include/asterisk/data.h 273879 
>   /trunk/include/asterisk/indications.h 273879 
>   /trunk/main/cdr.c 273879 
>   /trunk/main/channel.c 273879 
>   /trunk/main/data.c 273879 
>   /trunk/main/indications.c 273879 
>   /trunk/main/pbx.c 273879 
>   /trunk/res/res_odbc.c 273879 
> 
> Diff: https://reviewboard.asterisk.org/r/757/diff
> 
> 
> Testing
> -------
> 
> Developer testing.
> 
> 
> Thanks,
> 
> Eliel
> 
>