| Message ID | 20240322135439.11415-2-eric@garver.life |
|---|---|
| State | Changes Requested |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | dpif: probe support for OVS_ACTION_ATTR_DROP | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/22/24 14:54, Eric Garver wrote: > This is prep for adding a different OVS_ACTION_ATTR_ enum value. This > action, OVS_ACTION_ATTR_DEC_TTL, is not actually implemented. However, > to make -Werror happy we must add a case to all existing switches. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Eric Garver <eric@garver.life> > --- > include/linux/openvswitch.h | 1 + > lib/dpif-netdev.c | 1 + > lib/dpif.c | 1 + > lib/odp-execute.c | 2 ++ > lib/odp-util.c | 23 +++++++++++++++++++++++ > ofproto/ofproto-dpif-ipfix.c | 1 + > ofproto/ofproto-dpif-sflow.c | 1 + > 7 files changed, 30 insertions(+) > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h > index e305c331516b..a265e05ad253 100644 > --- a/include/linux/openvswitch.h > +++ b/include/linux/openvswitch.h > @@ -1085,6 +1085,7 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > + OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > #ifndef __KERNEL__ > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index e6c53937d8b9..89b0d1d6b4aa 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -9489,6 +9489,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > } > diff --git a/lib/dpif.c b/lib/dpif.c > index d07241f1e7cd..0f480bec48d0 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1289,6 +1289,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > } > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index eb03b57c42ec..081e4d43268a 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -837,6 +837,7 @@ requires_datapath_assistance(const struct nlattr *a) > case OVS_ACTION_ATTR_CT_CLEAR: > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case OVS_ACTION_ATTR_DROP: > return false; > > @@ -1227,6 +1228,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > case OVS_ACTION_ATTR_RECIRC: > case OVS_ACTION_ATTR_CT: > case OVS_ACTION_ATTR_UNSPEC: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > /* The following actions are handled by the scalar implementation. */ > case OVS_ACTION_ATTR_POP_VLAN: > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 9306c9b4d47a..f4c492f2ae6f 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -143,6 +143,7 @@ odp_action_len(uint16_t type) > case OVS_ACTION_ATTR_POP_NSH: return 0; > case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE; > case OVS_ACTION_ATTR_ADD_MPLS: return sizeof(struct ovs_action_add_mpls); > + case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE; > case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); > > case OVS_ACTION_ATTR_UNSPEC: > @@ -1130,6 +1131,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr, > ds_put_cstr(ds, "))"); > } > > +static void > +format_dec_ttl_action(struct ds *ds, const struct nlattr *attr, > + const struct hmap *portno_names) > +{ > + const struct nlattr *a; > + unsigned int left; > + > + ds_put_cstr(ds,"dec_ttl(le_1("); > + NL_ATTR_FOR_EACH (a, left, > + nl_attr_get(attr), nl_attr_get_size(attr)) { > + if (nl_attr_type(a) == OVS_ACTION_ATTR_DEC_TTL) { Hmm. This doesn't seem correct. There should be OVS_DEC_TTL_ATTR_ACTION instead of OVS_ACTION_ATTR_DEC_TTL. And we're missing the definition of the enum ovs_dec_ttl_attr in openvswitch.h. I know this implementation is not going to be actually used in most cases, unless someone manually adds the flow to the kernel, but we shouldn't have incorrect code anyway. > + format_odp_actions(ds, nl_attr_get(a), > + nl_attr_get_size(a), portno_names); > + break; > + } > + } > + ds_put_format(ds, "))"); > +} > + > static void > format_odp_action(struct ds *ds, const struct nlattr *a, > const struct hmap *portno_names) > @@ -1283,6 +1303,9 @@ format_odp_action(struct ds *ds, const struct nlattr *a, > ntohs(mpls->mpls_ethertype)); > break; > } > + case OVS_ACTION_ATTR_DEC_TTL: > + format_dec_ttl_action(ds, a, portno_names); > + break; > case OVS_ACTION_ATTR_DROP: > ds_put_cstr(ds, "drop"); > break; > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c > index e6c2968f7e90..cd65dae7e18a 100644 > --- a/ofproto/ofproto-dpif-ipfix.c > +++ b/ofproto/ofproto-dpif-ipfix.c > @@ -3135,6 +3135,7 @@ dpif_ipfix_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_UNSPEC: > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > default: > break; > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index a3c83bac8152..4a68e9b949b5 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -1236,6 +1236,7 @@ dpif_sflow_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > default: > break;
On Thu, Mar 28, 2024 at 11:13:13PM +0100, Ilya Maximets wrote: > On 3/22/24 14:54, Eric Garver wrote: > > This is prep for adding a different OVS_ACTION_ATTR_ enum value. This > > action, OVS_ACTION_ATTR_DEC_TTL, is not actually implemented. However, > > to make -Werror happy we must add a case to all existing switches. > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Signed-off-by: Eric Garver <eric@garver.life> > > --- > > include/linux/openvswitch.h | 1 + > > lib/dpif-netdev.c | 1 + > > lib/dpif.c | 1 + > > lib/odp-execute.c | 2 ++ > > lib/odp-util.c | 23 +++++++++++++++++++++++ > > ofproto/ofproto-dpif-ipfix.c | 1 + > > ofproto/ofproto-dpif-sflow.c | 1 + > > 7 files changed, 30 insertions(+) > > > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h > > index e305c331516b..a265e05ad253 100644 > > --- a/include/linux/openvswitch.h > > +++ b/include/linux/openvswitch.h > > @@ -1085,6 +1085,7 @@ enum ovs_action_attr { > > OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ > > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ > > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > > + OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > > > #ifndef __KERNEL__ > > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index e6c53937d8b9..89b0d1d6b4aa 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -9489,6 +9489,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, > > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > > case OVS_ACTION_ATTR_DROP: > > case OVS_ACTION_ATTR_ADD_MPLS: > > + case OVS_ACTION_ATTR_DEC_TTL: > > case __OVS_ACTION_ATTR_MAX: > > OVS_NOT_REACHED(); > > } > > diff --git a/lib/dpif.c b/lib/dpif.c > > index d07241f1e7cd..0f480bec48d0 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -1289,6 +1289,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, > > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > > case OVS_ACTION_ATTR_DROP: > > case OVS_ACTION_ATTR_ADD_MPLS: > > + case OVS_ACTION_ATTR_DEC_TTL: > > case __OVS_ACTION_ATTR_MAX: > > OVS_NOT_REACHED(); > > } > > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > > index eb03b57c42ec..081e4d43268a 100644 > > --- a/lib/odp-execute.c > > +++ b/lib/odp-execute.c > > @@ -837,6 +837,7 @@ requires_datapath_assistance(const struct nlattr *a) > > case OVS_ACTION_ATTR_CT_CLEAR: > > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > > case OVS_ACTION_ATTR_ADD_MPLS: > > + case OVS_ACTION_ATTR_DEC_TTL: > > case OVS_ACTION_ATTR_DROP: > > return false; > > > > @@ -1227,6 +1228,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > > case OVS_ACTION_ATTR_RECIRC: > > case OVS_ACTION_ATTR_CT: > > case OVS_ACTION_ATTR_UNSPEC: > > + case OVS_ACTION_ATTR_DEC_TTL: > > case __OVS_ACTION_ATTR_MAX: > > /* The following actions are handled by the scalar implementation. */ > > case OVS_ACTION_ATTR_POP_VLAN: > > diff --git a/lib/odp-util.c b/lib/odp-util.c > > index 9306c9b4d47a..f4c492f2ae6f 100644 > > --- a/lib/odp-util.c > > +++ b/lib/odp-util.c > > @@ -143,6 +143,7 @@ odp_action_len(uint16_t type) > > case OVS_ACTION_ATTR_POP_NSH: return 0; > > case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE; > > case OVS_ACTION_ATTR_ADD_MPLS: return sizeof(struct ovs_action_add_mpls); > > + case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE; > > case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); > > > > case OVS_ACTION_ATTR_UNSPEC: > > @@ -1130,6 +1131,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr, > > ds_put_cstr(ds, "))"); > > } > > > > +static void > > +format_dec_ttl_action(struct ds *ds, const struct nlattr *attr, > > + const struct hmap *portno_names) > > +{ > > + const struct nlattr *a; > > + unsigned int left; > > + > > + ds_put_cstr(ds,"dec_ttl(le_1("); > > + NL_ATTR_FOR_EACH (a, left, > > + nl_attr_get(attr), nl_attr_get_size(attr)) { > > + if (nl_attr_type(a) == OVS_ACTION_ATTR_DEC_TTL) { > > Hmm. This doesn't seem correct. There should be OVS_DEC_TTL_ATTR_ACTION > instead of OVS_ACTION_ATTR_DEC_TTL. And we're missing the definition > of the enum ovs_dec_ttl_attr in openvswitch.h. Right! Thanks for catching this. > I know this implementation is not going to be actually used in most cases, > unless someone manually adds the flow to the kernel, but we shouldn't have > incorrect code anyway. [..]
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h index e305c331516b..a265e05ad253 100644 --- a/include/linux/openvswitch.h +++ b/include/linux/openvswitch.h @@ -1085,6 +1085,7 @@ enum ovs_action_attr { OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ + OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ #ifndef __KERNEL__ OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e6c53937d8b9..89b0d1d6b4aa 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -9489,6 +9489,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); } diff --git a/lib/dpif.c b/lib/dpif.c index d07241f1e7cd..0f480bec48d0 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1289,6 +1289,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); } diff --git a/lib/odp-execute.c b/lib/odp-execute.c index eb03b57c42ec..081e4d43268a 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -837,6 +837,7 @@ requires_datapath_assistance(const struct nlattr *a) case OVS_ACTION_ATTR_CT_CLEAR: case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case OVS_ACTION_ATTR_DROP: return false; @@ -1227,6 +1228,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, case OVS_ACTION_ATTR_RECIRC: case OVS_ACTION_ATTR_CT: case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: /* The following actions are handled by the scalar implementation. */ case OVS_ACTION_ATTR_POP_VLAN: diff --git a/lib/odp-util.c b/lib/odp-util.c index 9306c9b4d47a..f4c492f2ae6f 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -143,6 +143,7 @@ odp_action_len(uint16_t type) case OVS_ACTION_ATTR_POP_NSH: return 0; case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE; case OVS_ACTION_ATTR_ADD_MPLS: return sizeof(struct ovs_action_add_mpls); + case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE; case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); case OVS_ACTION_ATTR_UNSPEC: @@ -1130,6 +1131,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr, ds_put_cstr(ds, "))"); } +static void +format_dec_ttl_action(struct ds *ds, const struct nlattr *attr, + const struct hmap *portno_names) +{ + const struct nlattr *a; + unsigned int left; + + ds_put_cstr(ds,"dec_ttl(le_1("); + NL_ATTR_FOR_EACH (a, left, + nl_attr_get(attr), nl_attr_get_size(attr)) { + if (nl_attr_type(a) == OVS_ACTION_ATTR_DEC_TTL) { + format_odp_actions(ds, nl_attr_get(a), + nl_attr_get_size(a), portno_names); + break; + } + } + ds_put_format(ds, "))"); +} + static void format_odp_action(struct ds *ds, const struct nlattr *a, const struct hmap *portno_names) @@ -1283,6 +1303,9 @@ format_odp_action(struct ds *ds, const struct nlattr *a, ntohs(mpls->mpls_ethertype)); break; } + case OVS_ACTION_ATTR_DEC_TTL: + format_dec_ttl_action(ds, a, portno_names); + break; case OVS_ACTION_ATTR_DROP: ds_put_cstr(ds, "drop"); break; diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index e6c2968f7e90..cd65dae7e18a 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -3135,6 +3135,7 @@ dpif_ipfix_read_actions(const struct flow *flow, case OVS_ACTION_ATTR_UNSPEC: case OVS_ACTION_ATTR_DROP: case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: default: break; diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index a3c83bac8152..4a68e9b949b5 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -1236,6 +1236,7 @@ dpif_sflow_read_actions(const struct flow *flow, case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: default: break;