[ovs-dev] [PATCH] ofproto-dpif: Issue clear error messages for unsupported CT features.

[Darrell Ball]

On 1/10/18, 1:05 PM, "Ben Pfaff" <blp at ovn.org> 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, "ovs-dev-bounces at openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces at openvswitch.org on behalf of blp at ovn.org> 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 <blp at ovn.org>
    >     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 <dlu998 at gmail.com>

Btw: All regression tests passed.