On Thu, Jul 06, 2023 at 10:40:18AM +0200, Eelco Chaudron wrote:
>
>
> On 30 Jun 2023, at 21:05, 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.
> >
> > Signed-off-by: Eric Garver <e...@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 | 2 ++
> > ofproto/ofproto-dpif-ipfix.c | 1 +
> > ofproto/ofproto-dpif-sflow.c | 1 +
> > 7 files changed, 9 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 abe63412ebfc..565cde5f5010 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -9193,6 +9193,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> > *packets_,
> > case OVS_ACTION_ATTR_POP_NSH:
> > case OVS_ACTION_ATTR_CT_CLEAR:
> > case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > + case OVS_ACTION_ATTR_DEC_TTL:
> > case OVS_ACTION_ATTR_DROP:
> > case OVS_ACTION_ATTR_ADD_MPLS:
>
> nit: I would add it here, to keep it in the same order ad the enum definition.
It can either be before _DROP or after _ADD_MPLS. In either case it's
out of order. :P
[..]
> > case __OVS_ACTION_ATTR_MAX:
> > diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> > index 37f0f717af61..86f1ccb39bf9 100644
> > --- a/lib/odp-execute.c
> > +++ b/lib/odp-execute.c
> > @@ -837,6 +837,7 @@ requires_datapath_assistance(const struct nlattr *a)
>
> I would put the below DEC_TTL here, although it’s not used due to design
> issues, but if it ever would be, it should be here.
ACK.
[..]
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 2ec889c417e5..e1ca2cd0e02b 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -146,6 +146,7 @@ odp_action_len(uint16_t type)
> > case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t);
> >
> > case OVS_ACTION_ATTR_UNSPEC:
> > + case OVS_ACTION_ATTR_DEC_TTL:
>
> This can be ‘case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE;’
ACK.
> > case __OVS_ACTION_ATTR_MAX:
> > return ATTR_LEN_INVALID;
> > }
> > @@ -1287,6 +1288,7 @@ format_odp_action(struct ds *ds, const struct nlattr
> > *a,
> > ds_put_cstr(ds, "drop");
> > break;
> > case OVS_ACTION_ATTR_UNSPEC:
> > + case OVS_ACTION_ATTR_DEC_TTL:
>
> I think we should be able to add the format for dec_ttl just in case it gets
> added through some other means.
>
> 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_DEC_TTL_ATTR_ACTION) {
> format_odp_actions(ds, nl_attr_get(a),
> nl_attr_get_size(a), portno_names);
> break;
> }
> }
> ds_put_format(ds, "))");
> }
Sure. I guess this will be compile only tested.
[..]
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
