| Message ID | 20161031213350.GD16705@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 31 October 2016 at 14:33, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Oct 31, 2016 at 02:16:05PM -0700, Joe Stringer wrote: >> On 31 October 2016 at 13:23, Ben Pfaff <blp@ovn.org> wrote: >> > Some datapaths do not support the ct action, and others support only a >> > subset of its features. Until now, it has been difficult to tell why a >> > particular action is being rejected. This commit should make it clearer. >> > >> > Reported-by: Kevin Lin <kevinlin@berkeley.edu> >> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-October/023060.html >> > Signed-off-by: Ben Pfaff <blp@ovn.org> >> >> Thanks, no doubt this will save a bunch of people a bunch of >> confusion. It still doesn't directly state that "Your kernel module >> may be out of date", but it's more clear than just OpenFlow hexdumps >> telling you "OFPBAC". Maybe it should either say this, or this should >> be mentioned in the FAQ. >> >> Acked-by: Joe Stringer <joe@ovn.org> > > You're right, this can be better. > > How about like this? > > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <blp@ovn.org> > Date: Mon, 31 Oct 2016 14:33:13 -0700 > Subject: [PATCH] ofproto-dpif: Log warning when ct action or its variants are > not supported. > > Some datapaths do not support the ct action, and others support only a > subset of its features. Until now, it has been difficult to tell why a > particular action is being rejected. This commit should make it clearer. > > Reported-by: Kevin Lin <kevinlin@berkeley.edu> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-October/023060.html > Signed-off-by: Ben Pfaff <blp@ovn.org> Looks good, thanks!
On Mon, Oct 31, 2016 at 05:23:25PM -0700, Joe Stringer wrote: > On 31 October 2016 at 14:33, Ben Pfaff <blp@ovn.org> wrote: > > On Mon, Oct 31, 2016 at 02:16:05PM -0700, Joe Stringer wrote: > >> On 31 October 2016 at 13:23, Ben Pfaff <blp@ovn.org> wrote: > >> > Some datapaths do not support the ct action, and others support only a > >> > subset of its features. Until now, it has been difficult to tell why a > >> > particular action is being rejected. This commit should make it clearer. > >> > > >> > Reported-by: Kevin Lin <kevinlin@berkeley.edu> > >> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-October/023060.html > >> > Signed-off-by: Ben Pfaff <blp@ovn.org> > >> > >> Thanks, no doubt this will save a bunch of people a bunch of > >> confusion. It still doesn't directly state that "Your kernel module > >> may be out of date", but it's more clear than just OpenFlow hexdumps > >> telling you "OFPBAC". Maybe it should either say this, or this should > >> be mentioned in the FAQ. > >> > >> Acked-by: Joe Stringer <joe@ovn.org> > > > > You're right, this can be better. > > > > How about like this? > > > > --8<--------------------------cut here-------------------------->8-- > > > > From: Ben Pfaff <blp@ovn.org> > > Date: Mon, 31 Oct 2016 14:33:13 -0700 > > Subject: [PATCH] ofproto-dpif: Log warning when ct action or its variants are > > not supported. > > > > Some datapaths do not support the ct action, and others support only a > > subset of its features. Until now, it has been difficult to tell why a > > particular action is being rejected. This commit should make it clearer. > > > > Reported-by: Kevin Lin <kevinlin@berkeley.edu> > > Reported-at: http://openvswitch.org/pipermail/discuss/2016-October/023060.html > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Looks good, thanks! Thanks, applied to master.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 7374ccc..330bd48 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4134,6 +4134,16 @@ check_mask(struct ofproto_dpif *ofproto, const struct miniflow *flow) return 0; } +static void +report_unsupported_ct(const char *detail) +{ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_WARN_RL(&rl, "Rejecting ct action because datapath does not support " + "ct action%s%s (your kernel module may be out of date)", + detail ? " " : "", + detail ? detail : ""); +} + static enum ofperr check_actions(const struct ofproto_dpif *ofproto, const struct rule_actions *const actions) @@ -4153,9 +4163,11 @@ check_actions(const struct ofproto_dpif *ofproto, support = &ofproto_dpif_get_support(ofproto)->odp; if (!support->ct_state) { + report_unsupported_ct(NULL); return OFPERR_OFPBAC_BAD_TYPE; } if ((ct->zone_imm || ct->zone_src.field) && !support->ct_zone) { + report_unsupported_ct("zone"); return OFPERR_OFPBAC_BAD_ARGUMENT; } @@ -4166,10 +4178,12 @@ check_actions(const struct ofproto_dpif *ofproto, /* The backer doesn't seem to support the NAT bits in * 'ct_state': assume that it doesn't support the NAT * action. */ + report_unsupported_ct("nat"); return OFPERR_OFPBAC_BAD_TYPE; } if (dst && ((dst->id == MFF_CT_MARK && !support->ct_mark) || (dst->id == MFF_CT_LABEL && !support->ct_label))) { + report_unsupported_ct("setting mark and/or label"); return OFPERR_OFPBAC_BAD_SET_ARGUMENT; } }