On Fri, May 17, 2013 at 01:40:10PM -0700, Jesse Gross wrote: > On Wed, May 15, 2013 at 1:31 AM, Simon Horman <ho...@verge.net.au> wrote: > > This moves generic action execution code out of lib/dpif-netedev.c > > and into a new file, lib/execute-actions.c. > > > > This is in preparation for using execute_set_action() > > in lib/odp-util.c to handle recirculation/ > > > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > Here's a combined set of comments for all of the patches in the > execute-actions code: > * The convention used other places in the code when passing around > void pointers and casting them is to have the variable named with an > underscore be the void *.
Thanks, I have fixed up the code to follow that convention. > * I would extract the userdata for userspace actions in > execute_actions() and pass it directly to the userspace function > pointer. Good idea, I have changed the patch accordingly. > * I don't think there is a lot of value in the asserts in > execute_actions(). I think it's valid to pass NULL for 'dp' and 'key' > in these situations if you want. The function pointers can't be NULL > but that will stop the program just as quickly as an assert anyways. Agreed. I have removed the asserts. > * The later patches pass around a huge number of arguments. We should > find some way of encapsulating them in a struct. It might even be > possible to store them directly in the flow. It deviates slightly from > other behavior because we don't update the flow when we change the > packet but it would clean a lot of other things up. I have been able update the patches to pass the parameters using the flow, reducing the number of new arguments. I have also squashed the patches that add skb_priority, skb_mark and tunnel support into a single patch which I think is easier to review. > * We already do some action execution in userspace when we send > packets to the controller. It would be good to see if we can start > using this code there as well. Thanks. I have added a patch to the series which implements this idea. Pleasingly the patch removes quite a bit of code which was previously only used for controller actions. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
