[asterisk-dev] [Code Review] Restore ao2 API compatibility and add new ao2_callback_data function

[Mark Michelson]

> On 2008-11-22 19:36:02, Sean Bright wrote:
> > /trunk/main/astobj2.c, lines 626-636
> > <http://reviewboard.digium.com/r/64/diff/1/?file=1603#file1603line626>
> >
> >     This is really the meat and potatoes of the diff.  Casting the void * to an appropriate function *.  I think it's safe, but there may be a better way.

I *think* the function pointer casting is fine, in that it makes sense to me. It would be nice to add in a check that could be executed at build time to be sure that if the type is WITH_DATA that the callback function is of the appropriate type, but I'm not sure if that's possible. As it is right now, I think that you could make a logic error (in that type is WITH_DATA but the cb_fn is of type ao2_callback_fn * instead of ao2_callback_data_fn *) but the compiler would not complain about it. Someone can correct me if this analysis is incorrect.

I'm concerned with the change in logic for how match is set. I know it's somewhat paranoid, but in the case that, for whatever reason, we decide to add some new CMP_* return value for the callback function, then it requires changing the initialization of match here. If you initialize match to 0 and just assign the return value of the callback function to the match variable, then no such precaution needs to be taken. Plus, I think the notation is clearer when no bitwise logic is used.


- Mark


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


On 2008-11-21 21:59:43, Sean Bright wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/64/
> -----------------------------------------------------------
> 
> (Updated 2008-11-21 21:59:43)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch, for all intents and purposes, simply reverts the majority of the ao2 changes that added the 'void *data' argument to the ao2_callback_fn, ao2_callback, and ao2_find calls.  It adds a new function, ao2_callback_data which takes the extra void *, and chan_sip is updated to use it (as this was the only place that was actually using the new argument).
> 
> Not included in this patch is documentation of the new function ao2_callback_data.  This will be added before commit.  Secondly, Russell suggested adding an ao2_find_data, but this is problematic since this calls the cmp callback on the container being passed.  I opted not to include it in this patch.
> 
> The real meat-and-potatoes to keep an eye on are the changes to main/astobj2.c.  The changes (casting a void * to the appropriate callback type) feels dirty, but it is necessary to avoid a good chunk of code duplication.
> 
> (As a total aside, for those of you having problems using post-review, I used `post-review --branch=/trunk --username=seanbright --password=mypassword` in my WC and it worked perfectly)
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_queue.c 158681 
>   /trunk/channels/chan_console.c 158681 
>   /trunk/channels/chan_iax2.c 158681 
>   /trunk/channels/chan_sip.c 158681 
>   /trunk/funcs/func_dialgroup.c 158681 
>   /trunk/include/asterisk/astobj2.h 158681 
>   /trunk/main/astobj2.c 158681 
>   /trunk/main/config.c 158681 
>   /trunk/main/features.c 158681 
>   /trunk/main/manager.c 158681 
>   /trunk/main/taskprocessor.c 158681 
>   /trunk/res/res_clialiases.c 158681 
>   /trunk/res/res_phoneprov.c 158681 
>   /trunk/res/res_timing_pthread.c 158681 
>   /trunk/res/res_timing_timerfd.c 158681 
>   /trunk/utils/hashtest2.c 158681 
> 
> Diff: http://reviewboard.digium.com/r/64/diff
> 
> 
> Testing
> -------
> 
> Compilation and developer tests.
> 
> 
> Thanks,
> 
> Sean
> 
>