On Wed, Jan 10, 2018 at 09:37:42PM +0000, Darrell Ball wrote: > > > On 1/10/18, 1:05 PM, "Ben Pfaff" <[email protected]> wrote: > > On Wed, Jan 10, 2018 at 08:49:15PM +0000, Darrell Ball wrote: > > Thanks for doing this; I have debugged the associated kinds of problems > and the new more granular errors for mask and action will help. > > > > I did not test it yet, but I have one comment inline. > > Thanks for the comments and the positive words! > > > On 1/10/18, 8:35 AM, "[email protected] on behalf of Ben > Pfaff" <[email protected] on behalf of [email protected]> wrote: > > > > I spent way too much time this last week tracking down errors due > to a > > VM with an antique kernel module that didn't support connection > tracking. > > This commit adds clear error messages that would have made the > problem > > obvious. > > > > Signed-off-by: Ben Pfaff <[email protected]> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 838a8de0c27f..da0c64671880 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -4193,12 +4193,16 @@ check_mask(struct ofproto_dpif *ofproto, > const struct miniflow *flow) > > support = &ofproto->backer->rt_support.odp; > > ct_state = MINIFLOW_GET_U8(flow, ct_state); > > > > + if (ct_state & CS_UNSUPPORTED_MASK) { > > + return OFPERR_OFPBMC_BAD_MASK; > > + } > > + > > > > [Darrell] Why do we still use OFPERR_OFPBMC_BAD_MASK above, rather > > than the new OFPERR_NXBMC_CT_DATAPATH_SUPPORT ? > > That's because this test is different from the others. Rather than > checking for a feature that is not supported in the datapath, it checks > whether the controller is trying to use a feature that has never been > defined and OVS doesn't know about. This might be caused, for example, > by a controller trying to use a connection tracking feature that only > exists in a newer version of OVS than the one running. > > I agree - it is a different error under the ‘CT class’ of errors. > > I guess we could invent another error code, something like > OFPBMC_UNKNOWN_CT_FEATURE, if we think that this is a likely problem. > > I don’t know how likely it is, but I would guess it can happen in general. > In this particular case, it is less likely, hence I am fine either way. > > Acked-by: Darrell Ball <[email protected]> > > Btw: All regression tests passed.
Thanks for the testing and thoughtful review. I applied this to master. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
