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

[Isaku Yamahata]

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