[asterisk-dev] [Code Review] added AES_ENCRYPT and AES_DECRYPT dialplan functions

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/128/#review310
-----------------------------------------------------------



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment700>

    Trim the trailing whitespace.
    
    In vim ...
    
    :%s/\s\+$//g



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment701>

    I wouldn't put a space after the comma in this log message.  It does not get automatically stripped out.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment703>

    Instead of doing a memset, you can have the data initialized for you when you declare it:
    
    unsigned char curblock[16] = { 0, };
    
    The C standard says that the rest of the array is automatically guaranteed to be initialized to 0 if any part of it is initialized.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment702>

    We should probably issue a warning if we have to truncate the data.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment707>

    spaces around the '-', and no parens necessary here



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment705>

    Take a look at what happens in this loop if the input data size is not evenly divisible by 16.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment704>

    You can use memcpy() here



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment706>

    Most of my comments for the encrypt function apply to this one as well.
    
    Another thing to consider is whether it would be worth it to merge encrypt/decrypt into a single function.  The code is just so similar for both.  YOu can use the "cmd" argument to determine which operation you should perform.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment708>

    You can just do res | ..., instead of |= since you don't need to store the result for any reason



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment709>

    In this case, you shouldn't return this directly.  You should do ...
    
    return res ? AST_MODULE_LOAD_DECLINE : AST_MODULE_LOAD_SUCCESS;
    
    Not all modules do this properly, but returning these codes directly is what is the right thing to do.


- Russell


On 2009-01-22 11:57:00, dvossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/128/
> -----------------------------------------------------------
> 
> (Updated 2009-01-22 11:57:00)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Mark Michelson.
> 
> 
> Summary
> -------
> 
> Allows data to be encrypted and decrypted using AES in the dialplan.  
> 
> 
> This addresses bug 0014301.
>     http://bugs.digium.com/view.php?id=0014301
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_aes.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/128/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> dvossel
> 
>