[ovs-dev] [PATCH v5 2/6] instruction: support goto-table action

[Ben Pfaff]

Thanks!

On Fri, Aug 31, 2012 at 03:48:22AM +0900, Isaku Yamahata wrote:
> Thank you for review. I'll try to simplify the logic.
> Since I don't have strong opinion for textual form, I'll change its
> syntax as you suggest.
> 
> thanks,
> 
> On Tue, Aug 28, 2012 at 09:05:33PM -0700, Ben Pfaff wrote:
> > On Wed, Aug 29, 2012 at 02:19:01AM +0900, Isaku Yamahata wrote:
> > > Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
> > 
> > I'm not sure why you're verifying that padding in instructions is
> > all-zero.  (Is that something that I suggested?)  I don't see anything
> > in OF1.1 that says that the padding fields must be zero, so it seems
> > inappropriate to insist that they be zero.
> > 
> > I find the ofpacts_put_openflow11_instructions() function hard to
> > follow.  Without further study, I'm not sure that it is correct.  I
> > think that it would be better to use a slightly different structure,
> > where a loop find the boundaries of the current instruction, whether
> > that instruction consists of a sequence of actions or a single
> > non-action instruction, and then process that after the boundaries have
> > been found.  Did you consider such a structure?
> > 
> > The textual form of instructions isn't what I was envisioning.  Here's
> > what I'd pictured a set of instructions would look like (adding extra
> > spaces):
> > 
> >         actions=mod_vlan_vid:123, output:10, write_metadata(...),
> >         goto_table(...)
> > 
> > I think that in the syntax that you are proposing, it would look like
> > this:
> > 
> >         instructions=apply_actions(mod_vlan_vid:123, output:10),
> >         write_metadata(...), goto_table(...)
> > 
> > The difference is, then, that "actions" becomes "instructions" and the
> > "apply_actions" keyword becomes required.  I'm not sure of the benefit,
> > and the result is much longer.  What do you think?
> > 
> > I'd move the str_to_inst_ofpacts() calls to ovs_fatal() into the switch
> > statement in parse_named_instruction().
> > 
> > I get a test failure:
> > 
> > #                             -*- compilation -*-
> > 66. ofp-actions.at:245: testing OpenFlow 1.1 instruction translation ...
> > ../../tests/ofp-actions.at:323: ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-ofp11-instructions < input.txt
> > --- expout      2012-08-28 20:36:00.000000000 -0700
> > +++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/66/stdout 2012-08-28 20:36:00.000000000 -0700
> > @@ -30,7 +30,7 @@
> >  
> >  bad OF1.1 instructions: OFPBIC_UNSUP_INST
> >  
> > -instructions=clear_actions
> > +bad OF1.1 instructions: OFPBIC_UNSUP_INST
> >  
> >  bad OF1.1 instructions: OFPBIC_BAD_EXPERIMENTER
> >  
> > Thanks,
> > 
> > Ben.
> > 
> 
> -- 
> yamahata