On Tue, Jul 24, 2012 at 10:17:11PM -0700, Ben Pfaff wrote:
> 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.
Thank you for review. I'll reorganize patch series based on the latter
approach and address the reviews.
--
yamahata
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
