[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.
> 
>  wrote:
>     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.
> 
>  wrote:
>     Well, you're right that the compiler would not complain.  I will look at the GCC docs/mailing list to see if there is a way to check that stuff at runtime, or Kevin can pipe in because he's the GCC guru.
>     
>     As for the "change in logic" I don't know what you mean.  The original code bitwise and-ed the result of the callback with (CMP_MATCH | CMP_STOP) as well, I just moved that to a single place instead of duplicating it with each function call, or am I not seeing something?
> 
>  wrote:
>     And re: function pointer type safety - __ao2_callback is not callable from outside astobj2.c, and the delegating functions in astobj2.c both have the correct type information for their respective callbacks (ao2_callback_fn and ao2_callback_data_fn).  So the only way this would be a problem would be if you were adding code into astobj.c itself.

Okay, I missed the fact that the result was originally bitwise anded with (CMP_MATCH | CMP_STOP). And with the last note you made, it sounds like everything's cool. I need to be more careful and look at the *entire* diff instead of just the "interesting" part so I don't make a blunder like the type-checking remark in my previous comment. Consider this a "Ship it!" from me.


- 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
> 
>