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

Ben Pfaff blp at nicira.com
Wed Aug 29 04:05:33 UTC 2012


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.



More information about the dev mailing list