[asterisk-dev] [Code Review] awesome review

[Russell Bryant]

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



/branches/1.8/funcs/func_awesome_trace.c
<https://reviewboard.asterisk.org/r/925/#comment5926>

    You can simplify things a bit by doing something like:
    
     ... = {
        [AST_FRAME_DTMF_BEGIN] = "DTMF_BEGIN",
    }
    
    That way, you can use the frame type as the index into the array instead of having to iterate to find what you need for every frame.



/branches/1.8/funcs/func_awesome_trace.c
<https://reviewboard.asterisk.org/r/925/#comment5927>

    coding guidelines, { on next line



/branches/1.8/include/asterisk/awesomehook.h
<https://reviewboard.asterisk.org/r/925/#comment5920>

    channel.h and audiohook.h include each other.  Is this needed?  I think you can remove channel.h from audiohook.h to resolve the circular reference.  The most common data structures (ast_channel and ast_frame, for example) are declared in asterisk.h, so you can reference them by name safely, you just can't dereference them.
    



/branches/1.8/include/asterisk/awesomehook.h
<https://reviewboard.asterisk.org/r/925/#comment5919>

    For potentially improving API and ABI stability, I'd like to explore a slightly different approach to the arguments to ast_awesomehook_attach().  Right now, if anything needs to be added to what gets registered, it changes the API.  Here is an alternative type approach that I have in mind.  I don't know how to explain it any easier than just code ...
    
    BEGIN CODE
    
    struct ast_awesomehook_interface {
        uint32_t version;
        #define AST_AWESOMEHOOK_INTERFACE_VERSION 1
        ast_awesomehook_event_callback event_cb;
        ast_awesomehook_destroy_callback destroy_cb;
        void *stored_data;
    };
    
    struct ast_awesomehook *ast_awesomehook_attach(struct ast_channel *chan, struct ast_awesomehook_interface *i);
    
    void some_function(void)
    {
        ...
    
        struct ast_awesomehook_interface i = {
            .version = AST_AWESOMEHOOK_INTERFACE_VERSION,
            ...
        };
    
        ...
    
        ast_awesomehook_attach(chan, &i);
    
        ...
    }
    
    END CODE
    
    By using a structure with a version, additional elements can be added if needed without breaking the API.  For ABI compatibility, the consumer of the structure (in this case, the Asterisk core) can check the version number to know what version of the structure the producer was compiled against.



/branches/1.8/include/asterisk/awesomehook.h
<https://reviewboard.asterisk.org/r/925/#comment5922>

    It would be good to add "\since 1.8" to all the docs of these new API calls.



/branches/1.8/include/asterisk/awesomehook.h
<https://reviewboard.asterisk.org/r/925/#comment5921>

    The doxygen element \pre would be good for this.  It stands for precondition.



/branches/1.8/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/925/#comment5923>

    One thing that's nice about the use of a pointer to an audiohook_list instead of an instance of an audiohook_list is that it can be made an opaque type.  Besides, the majority of channels don't need an awesomehook (OR DO THEY?!  ;-) ), so it's probably more efficient to just allocate the list structure once it's needed.
    
    This alternative approach would also allow changing how the ast_awesomehook_list is implemented without changing the ABI.



/branches/1.8/main/awesomehook.c
<https://reviewboard.asterisk.org/r/925/#comment5924>

    doxygenify



/branches/1.8/main/awesomehook.c
<https://reviewboard.asterisk.org/r/925/#comment5925>

    As an example benefit of just passing back an ast_audiohook handle instead of an integer id, this function would be greatly simplified.  Instead of having to traverse the list of hooks to find it, you can directly set this flag instead.


- Russell


On 2010-09-17 11:37:55, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/925/
> -----------------------------------------------------------
> 
> (Updated 2010-09-17 11:37:55)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> I wrote a new API called AwesomeHook.  It allows for ast_frames to be intercepted, viewed, and modified while being read and written to a channel.  Detailed documentation pertaining to this API can be found in awesomehook.h.  I have also included a new dialplan function called AWESOME_TRACE() which exercises the AwesomeHook API.  Documentation and use of this function can be found in func_awesome_trace.c.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/funcs/func_awesome_trace.c PRE-CREATION 
>   /branches/1.8/include/asterisk/awesomehook.h PRE-CREATION 
>   /branches/1.8/include/asterisk/channel.h 287384 
>   /branches/1.8/main/awesomehook.c PRE-CREATION 
>   /branches/1.8/main/channel.c 287384 
> 
> Diff: https://reviewboard.asterisk.org/r/925/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David
> 
>