On Fri, May 03, 2013 at 09:38:29AM -0700, Ben Pfaff wrote:
> 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 <[email protected]>
> > Signed-off-by: Simon Horman <[email protected]>
>
> 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?
Sure, I'll see what I can do.
> 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.
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
