[ovs-dev] [PATCH] Do not perform validation of learn actions during parsing

Ben Pfaff blp at nicira.com
Fri May 3 16:38:29 UTC 2013


On Thu, May 02, 2013 at 06:06:35PM +0900, Simon Horman wrote:
> Do not perform validation of learn actions during parsing.
> I believe this is consistent with the handling of all other actions.
> 
> Verification of all actions, including learn actions, occurs separately
> in ofpact_check__().
> 
> This the code portion of this patch is larger than might otherwise be
> expected as the flow argument of learn_parse() is now unused and thus
> removed.  This propagates up the call-chain some way.
> 
> This was suggested by Jesse Gross in response to an enhancement
> I made to the validation performed during parsing learn actions
> to allow it to correctly account for changes to the dl_type
> due to MPLS push and pop actions.
> 
> Tests have also been updated to use ovs-ofctl add-flow* instead
> of ovs-ofctl parse-flow to to allow verification occur using
> ofpact_check__().
> 
> Cc: Jesse Gross <jesse at nicira.com>
> Signed-off-by: Simon Horman <horms+renesas at verge.net.au>

The problem with this change is that it makes the error messages
impossible to read.  Just look at the learn.at update from your diff.
It changes this error message:

  ovs-ofctl: load:5->NXM_OF_IP_DST[]: cannot specify destination field
  ip_dst because prerequisites are not satisfied

into this one:

  OFPT_ERROR (xid=0x2): OFPBAC_BAD_ARGUMENT
  OFPT_FLOW_MOD (xid=0x2):
  (***truncated to 64 bytes from 120***)
  00000000  01 0e 00 78 00 00 00 02-00 38 20 ff 00 00 00 00 |...x.....8 .....|
  00000010  00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 |................|
  00000020  00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 |................|
  00000030  00 00 00 00 00 00 00 00-00 00 00 00 00 00 80 00 |................|

I know we'll get complaints about that.

Here's a change to ofp-parse.c that changes the error message back
into:

  2013-05-03T16:34:21Z|00001|meta_flow|WARN|destination field ip_dst lacks correct prerequisites

which is less specific than before but at least hints at the problem.

Can you please work that in?

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index fce0765..c55bf5f 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1009,6 +1009,8 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
         str_to_inst_ofpacts(act_str, &ofpacts);
         fm->ofpacts_len = ofpacts.size;
         fm->ofpacts = ofpbuf_steal_data(&ofpacts);
+
+        ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow, OFPP_MAX);
     } else {
         fm->ofpacts_len = 0;
         fm->ofpacts = NULL;

Thanks,

Ben.



More information about the dev mailing list