[asterisk-dev] [Code Review] Generic Advice of Charge

[David Vossel]

> On 2010-04-15 16:35:24, rmudgett wrote:
> > /team/rmudgett/aoc_event/main/aoc.c, lines 249-251
> > <https://reviewboard.asterisk.org/r/552/diff/3/?file=9255#file9255line249>
> >
> >     This decoding of a multi-byte integer is not reliable.  What if len were something other than 4?  A helper routine to decode an integer would be a more robust method.  The ntohl() is only valid if the size is as expected.
> >     
> >     There are integer encode/decode routines in the libpri ASN.1 code which could be extracted and made into utility routines for the Asterisk control frames.
> 
> David Vossel wrote:
>     ie.amount is set as a uint32_t.  Does that not guarantee it is 4 bytes?
> 
> rmudgett wrote:
>     I suppose that 4 bytes could be a requirement of the sender.  However, a long is only guaranteed to be a minimum of 32 bits.
>     
>     More subtlely, you are checking to make sure that the struct length is as expected.  However, you are depending upon the size of struct aoc_ie_currency to be the same on all platforms.  You are not taking into account that padding bytes are possible and likely to affect the size.
>     
>     #define AOC_CURRENCY_NAME_SIZE (10 + 1)
>     struct aoc_ie_currency {
>     	uint8_t  multiplier;
>     	uint32_t amount;
>     	char name[AOC_CURRENCY_NAME_SIZE];
>     };
>     
>     There is likely to be 3 bytes of padding after multiplier.  There is likely to be a padding byte after name to bring the struct up to a multiple of uint32_t for alignment purposes.
> 
> David Vossel wrote:
>     good point, I'll have to break up some of these into more IE's then.
> 
> Shaun Ruffell wrote:
>     If it makes things easier:  ntohl typically takes and receives a uint32_t because it was originally written when a long was 4 bytes, so you could just:
>     
>     struct aoc_ie_currency {
>         uint8_t multiplier;
>         uint32_t amount;
>         char name[AOC_CURRENCY_NAME_SIZE];
>     } __attribute__((packed));
>     
>     
>     And then share it between machines without worrying about padding.  A potential option if it's easier then breaking it up.

Thanks Shaun, I think that's the best solution.


- David


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


On 2010-04-13 17:43:42, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/552/
> -----------------------------------------------------------
> 
> (Updated 2010-04-13 17:43:42)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Asterisk Generic AOC Representation
> - Generic AOC encode/decode routines. (Generic AOC must be encoded to be passed on the wire in the AST_CONTROL_AOC frame)
> - AST_CONTROL_AOC frame type to represent generic encoded AOC data
> - Manager events for AOC-S, AOC-D, and AOC-E messages
> 
> Asterisk App Support
> - app_dial AOC-S pass-through support on call setup
> - app_queue AOC-S pass-through support on call setup
> 
> AOC Unit Tests
> - AOC Unit Tests for encode/decode routines
> - AOC Unit Test for manager event representation.
> 
> SIP AOC Support
> - Pass-through of generic AOC-D and AOC-E messages to snom phones via the snom AOC specification.
> - Creation of chan_sip page3 flags for the addition of the new 'snom_aoc_enabled' sip.conf option.
> 
> IAX AOC Support
> - Natively supports AOC pass-through through the use of the new AST_CONTROL_AOC frame type
> 
> DAHDI AOC Support
> - ETSI PRI full AOC Pass-through support
> - 'aoc_enable' chan_dahdi.conf option for independently enabling pass-through of AOC-S, AOC-D, AOC-E.
> - 'aoce_delayhangup' option for retrieving AOC-E on disconnect.
> - DAHDI A() dial string option for requesting AOC services.
>   example usage: exten=>1111,1,Dial(DAHDI/g1/1112/A(s,d,e)) ;requests AOC-S, AOC-D, and AOC-E on call setup
> 
> 
> Diffs
> -----
> 
>   /team/rmudgett/aoc_event/apps/app_dial.c 257246 
>   /team/rmudgett/aoc_event/apps/app_queue.c 257246 
>   /team/rmudgett/aoc_event/channels/chan_dahdi.c 257246 
>   /team/rmudgett/aoc_event/channels/chan_sip.c 257246 
>   /team/rmudgett/aoc_event/channels/sig_pri.h 257246 
>   /team/rmudgett/aoc_event/channels/sig_pri.c 257246 
>   /team/rmudgett/aoc_event/channels/sip/include/sip.h 257246 
>   /team/rmudgett/aoc_event/configs/chan_dahdi.conf.sample 257246 
>   /team/rmudgett/aoc_event/configs/sip.conf.sample 257246 
>   /team/rmudgett/aoc_event/include/asterisk/aoc.h PRE-CREATION 
>   /team/rmudgett/aoc_event/include/asterisk/frame.h 257246 
>   /team/rmudgett/aoc_event/main/aoc.c PRE-CREATION 
>   /team/rmudgett/aoc_event/main/asterisk.c 257246 
>   /team/rmudgett/aoc_event/main/channel.c 257246 
>   /team/rmudgett/aoc_event/main/features.c 257246 
>   /team/rmudgett/aoc_event/main/manager.c 257246 
>   /team/rmudgett/aoc_event/tests/test_aoc.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/552/diff
> 
> 
> Testing
> -------
> 
> I have tested every feature listed in the description to some extent, but only back to back with Asterisk.   I am going to need some community help to test interoperability.
> 
> To test this code, check out both my Asterisk and Libpri changes.
> 
> Asterisk Changes: svn/asterisk/team/dvossel/generic_aoc
> LibPRI Changes: svn/libpri/team/dvossel/aoc_send 
> 
> 
> Thanks,
> 
> David
> 
>