[ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute

[Eric Garver]

On Mon, Jul 10, 2017 at 09:30:22AM +0000, Zoltán Balogh wrote:
> Hi Eric,
> 
> Thank you for fixing this. I'm fine with your solution. Tests do pass.

Thanks for verifying. I'll add the patches below to my series and post
it ASAP.

> 
> Best regards,
> Zoltan
> 
> > This still did not do the trick. The ETHERTYPE is placed at the same
> > level of the message as OVS_FLOW_ATTR_MASK, rather than being place
> > _inside_ OVS_FLOW_ATTR_MASK.
> > 
> > Below is what I came up with. All test cases pass unchanged. It also
> > works for my series to add kernel L3 VXLAN-GPE/GRE.
> > 
> > https://github.com/erig0/ovs/commits/46aafc3ad12e30772f9d8561e6750e6c6334077c
> > 
> > Let me know how it works for you.
> > 
> > Eric.
> > 
> > --->8---
> > 
> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > index 562f6134c3a5..0654ba1b2131 100644
> > --- a/lib/dpif-netlink.c
> > +++ b/lib/dpif-netlink.c
> > @@ -3434,31 +3434,47 @@ dpif_netlink_flow_from_ofpbuf(struct dpif_netlink_flow *flow,
> > 
> > 
> >  /*
> > - * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out,
> > - * then puts 'data' to 'buf'.
> > + * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out.
> > + * If the flow is not Ethernet, packet_type is converted to ETHERTYPE.
> > + * Puts 'data' to 'buf'.
> >   */
> >  static void
> >  put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
> > -                        const struct nlattr *data, uint16_t data_len)
> > +                        const struct nlattr *data, size_t data_len)
> >  {
> >      const struct nlattr *packet_type;
> > 
> >      packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE);
> > 
> >      if (packet_type) {
> > -        /* exclude PACKET_TYPE Netlink attribute. */
> > -        ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE);
> > -        size_t packet_type_len = NL_A_U32_SIZE;
> > -        size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data;
> > -        size_t second_chunk_size = data_len - first_chunk_size
> > -                                   - packet_type_len;
> > -        uint8_t *first_attr = NULL;
> > -        struct nlattr *next_attr = nl_attr_next(packet_type);
> > -
> > -        first_attr = nl_msg_put_unspec_uninit(buf, type,
> > -                                              data_len - packet_type_len);
> > -        memcpy(first_attr, data, first_chunk_size);
> > -        memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size);
> > +        struct odputil_keybuf new_attrs_buf;
> > +        struct nlattr *new_attrs;
> > +        const struct nlattr *ethernet;
> > +        size_t new_attrs_len;
> > +        ovs_be32 packet_type_value = nl_attr_get_be32(packet_type);
> > +
> > +        new_attrs = (struct nlattr *) &new_attrs_buf;
> > +        memcpy(new_attrs, data, data_len);
> > +        new_attrs_len = data_len;
> > +
> > +        nl_attrs_filter(new_attrs, &new_attrs_len, OVS_KEY_ATTR_PACKET_TYPE);
> > +
> > +        /* If it's not Ethernet, then we need to override or add the Ethertype
> > +         * with the one from the packet_type. */
> > +        ethernet = nl_attr_find__(data, data_len, OVS_KEY_ATTR_ETHERNET);
> > +        if (!ethernet) {
> > +            struct ofpbuf new_buf;
> > +
> > +            nl_attrs_filter(new_attrs, &new_attrs_len, OVS_KEY_ATTR_ETHERTYPE);
> > +
> > +            ofpbuf_use_stack(&new_buf, new_attrs, sizeof new_attrs_buf);
> > +            nl_msg_put_uninit(&new_buf, new_attrs_len);
> > +            nl_msg_put_be16(&new_buf, OVS_KEY_ATTR_ETHERTYPE,
> > +                            pt_ns_type_be(packet_type_value));
> > +            new_attrs_len = new_buf.size;
> > +        }
> > +
> > +        nl_msg_put_unspec(buf, type, new_attrs, new_attrs_len);
> >      } else {
> >          nl_msg_put_unspec(buf, type, data, data_len);
> >      }
> > diff --git a/lib/netlink.c b/lib/netlink.c
> > index 4cf1aaca621c..c9b83fc681c4 100644
> > --- a/lib/netlink.c
> > +++ b/lib/netlink.c
> > @@ -927,3 +927,23 @@ nl_attr_find_nested(const struct nlattr *nla, uint16_t type)
> >  {
> >      return nl_attr_find__(nl_attr_get(nla), nl_attr_get_size(nla), type);
> >  }
> > +
> > +/*
> > + * Filter nlattr type from set of nlattrs.
> > + * This changes the data in place. So caller should make a copy if required.
> > + */
> > +void
> > +nl_attrs_filter(struct nlattr *attrs, size_t *attrs_len, uint16_t type)
> > +{
> > +    size_t size = *attrs_len;
> > +    struct nlattr *nla;
> > +    size_t left;
> > +
> > +    NL_ATTR_FOR_EACH (nla, left, attrs, size) {
> > +        if (nl_attr_type(nla) == type) {
> > +            *attrs_len -= nl_attr_len_pad(nla, left);
> > +            memmove(nla, nl_attr_next(nla), left - nl_attr_len_pad(nla, left));
> > +            return;
> > +        }
> > +    }
> > +}
> > diff --git a/lib/netlink.h b/lib/netlink.h
> > index 6dfac27c9d4b..744c9520966f 100644
> > --- a/lib/netlink.h
> > +++ b/lib/netlink.h
> > @@ -240,5 +240,6 @@ const struct nlattr *nl_attr_find(const struct ofpbuf *, size_t hdr_len,
> >  const struct nlattr *nl_attr_find_nested(const struct nlattr *, uint16_t type);
> >  const struct nlattr *nl_attr_find__(const struct nlattr *attrs, size_t size,
> >                                      uint16_t type);
> > +void nl_attrs_filter(struct nlattr *attrs, size_t *attrs_len, uint16_t type);
> > 
> >  #endif /* netlink.h */
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev