On Mon, Jul 23, 2012 at 05:08:39PM +0900, Isaku Yamahata wrote:
> This patch adds instruction OF11 apply-actions/goto-table supports.
> 
> Okay, I re-write the patch series by introducing
> OFPACT_{CLEAR_ACTIONS, WRITE_ACTIONS, GOTO_TABLE}.
> I did only compile-tested for now. If this approach is okay, I'll completes
> the patch series and post it.

So, after having reviewed it, my major comment is that it appears that
the initial patches introduce a bunch of stuff that could cause aborts
at runtime due to NOT_REACHED(), and then the later patches fix those up
one by one.  I don't really like that kind of approach, because it means
that the intermediate commits are buggy.

I'd suggest one of two approaches:

        - Fold together all the commits from the point where the
          NOT_REACHED()s are introduced to the point where they are
          fixed.

        - Instead of breaking up patches by area (e.g. decoding,
          encoding, formatting, parsing), break them up by instruction
          (e.g. one patch for Clear-Actions, one for Goto-Table, and so
          on).

Normally I'd insist on the latter but this stuff appears pretty
straightforward so I don't think it's a big deal.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to