On Mon, Jun 24, 2013 at 12:29:55PM -0700, Jesse Gross wrote: > On Sun, Jun 23, 2013 at 7:29 PM, Simon Horman <ho...@verge.net.au> wrote: > > It appears that in the case where vs_match_init() is called from > > ovs_flow_metadata_from_nlattrs() it is undesirable to set the flow key as > > some of its values are set earlier on in ovs_flow_metadata_from_nlattrs(). > > Furthermore in the case where ovs_flow_metadata_from_nlattrs() is called > > from ovs_packet_cmd_execute() key has been partially initialised by > > ovs_flow_extract(). > > > > This manifests in a problem when executing actions as elements of key are > > used when verifying some actions. For example a dec_ttl action verifies the > > proto of the flow. An example of a flow that fails as a result of this > > problem is: > > > > ovs-ofctl add-flow br0 "ip actions=dec_ttl,normal" > > > > This patch resolves this problem by not clearing key in ovs_match_init() > > and instead clearing it before calling ovs_match_init() when it is called > > other than by ovs_flow_metadata_from_nlattrs(). > > > > This appears to be a regression added by "datapath: Mega flow > > implementation", a1c564be1e2ffc31f8da09ab654c8ed987907fe5. > > > > Cc: Andy Zhou <az...@nicira.com> > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > > > --- > > > > I am unsure of the exact intention of clearing the key and thus the > > correctness of this patch. However, it does appear to resolve > > the problem that I describe above. > > Thanks, I think this can simplified a little bit. I applied the following > patch:
Thanks, I have confirmed that resolves the problem that I observed. > commit c121c7ce259016fb5630c382bf99f9ac4d5900fc > Author: Jesse Gross <je...@nicira.com> > Date: Mon Jun 24 12:21:29 2013 -0700 > > datapath: Do not clear key in ovs_match_init() > > When executing packets sent from userspace, the majority of the > flow information is extracted from the packet itself and a small > amount of metadata supplied by userspace is added. However, when > adding this metadata, the extracted flow information is currently > being cleared. > > This manifests in a problem when executing actions as elements of key are > used when verifying some actions. For example a dec_ttl action verifies th > proto of the flow. An example of a flow that fails as a result of this > problem is: > > ovs-ofctl add-flow br0 "ip actions=dec_ttl,normal" > > This is a regression added by "datapath: Mega flow implementation", > a1c564be1e2ffc31f8da09ab654c8ed987907fe5. > > CC: Andy Zhou <az...@nicira.com> > Reported-by: Simon Horman <ho...@verge.net.au> > Signed-off-by: Jesse Gross <je...@nicira.com> > > diff --git a/datapath/flow.c b/datapath/flow.c > index 499e3e2..39de931 100644 > --- a/datapath/flow.c > +++ b/datapath/flow.c > @@ -1601,7 +1601,8 @@ int ovs_flow_metadata_from_nlattrs(struct sw_flow *flow, > if (err) > return -EINVAL; > > - ovs_match_init(&match, &flow->key, NULL); > + memset(&match, 0, sizeof(match)); > + match.key = &flow->key; > > err = metadata_from_nlattrs(&match, &attrs, a, false); > if (err) > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
