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 <yamah...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
