[ovs-dev] [PATCH] odp-util: Print eth() for Ethernet flows if packet_type is absent.

[Ben Pfaff]

Thanks, I applied this to master, branch-2.9, and branch-2.8.

On Wed, Mar 14, 2018 at 03:48:26PM -0700, Yi-Hung Wei wrote:
> Thanks for the fix. I tested this patch, and it does show eth() when
> the OVS_KEY_ATTR_ETHERNET is all-wildcarded.
> 
> Acked-by: Yi-Hung Wei <yihung.wei at gmail.com>
> 
> On Wed, Mar 14, 2018 at 2:57 PM, Ben Pfaff <blp at ovn.org> wrote:
> > OVS datapaths have two different ways to indicate what kind of packet a
> > flow matches.  One way, used by the userspace datapath, is
> > OVS_KEY_ATTR_PACKET_TYPE.  Another way, used by the kernel datapath, is
> > OVS_KEY_ATTR_ETHERTYPE when used in the absence of OVS_KEY_ATTR_ETHERNET;
> > when the latter is present, the packet is always an Ethernet packet.  The
> > code to print datapath flows wasn't paying attention to this distinction
> > and always omitted eth() from the output when OVS_KEY_ATTR_ETHERNET was
> > fully wildcarded, which meant that upon later re-parsing the
> > OVS_KEY_ATTR_ETHERNET key was omitted, which made it look like a
> > non-Ethernet match was being described.
> >
> > This commit makes odp_util_format() add eth() to the output when
> > OVS_KEY_ATTR_ETHERNET is present and OVS_KEY_ATTR_PACKET_TYPE is absent,
> > avoiding the problem.
> >
> > Reported-by: Amar Padmanabhan <amarpadmanabhan at fb.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html
> > Reported-by: Su Wang <suwang at vmware.com>
> > VMWare-BZ: #2070488
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> >  lib/odp-util.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 5da83b4c64c4..874350325050 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -4008,6 +4008,7 @@ odp_flow_format(const struct nlattr *key, size_t key_len,
> >          const struct nlattr *a;
> >          unsigned int left;
> >          bool has_ethtype_key = false;
> > +        bool has_packet_type_key = false;
> >          struct ofpbuf ofp;
> >          bool first_field = true;
> >
> > @@ -4028,6 +4029,8 @@ odp_flow_format(const struct nlattr *key, size_t key_len,
> >
> >              if (attr_type == OVS_KEY_ATTR_ETHERTYPE) {
> >                  has_ethtype_key = true;
> > +            } else if (attr_type == OVS_KEY_ATTR_PACKET_TYPE) {
> > +                has_packet_type_key = true;
> >              }
> >
> >              is_nested_attr = odp_key_attr_len(ovs_flow_key_attr_lens,
> > @@ -4050,6 +4053,34 @@ odp_flow_format(const struct nlattr *key, size_t key_len,
> >                  }
> >                  format_odp_key_attr__(a, ma, portno_names, ds, verbose);
> >                  first_field = false;
> > +            } else if (attr_type == OVS_KEY_ATTR_ETHERNET
> > +                       && !has_packet_type_key) {
> > +                /* This special case reflects differences between the kernel
> > +                 * and userspace datapaths regarding the root type of the
> > +                 * packet being matched (typically Ethernet but some tunnels
> > +                 * can encapsulate IPv4 etc.).  The kernel datapath does not
> > +                 * have an explicit way to indicate packet type; instead:
> > +                 *
> > +                 *   - If OVS_KEY_ATTR_ETHERNET is present, the packet is an
> > +                 *     Ethernet packet and OVS_KEY_ATTR_ETHERTYPE is the
> > +                 *     Ethertype encoded in the Ethernet header.
> > +                 *
> > +                 *   - If OVS_KEY_ATTR_ETHERNET is absent, then the packet's
> > +                 *     root type is that encoded in OVS_KEY_ATTR_ETHERTYPE
> > +                 *     (i.e. if OVS_KEY_ATTR_ETHERTYPE is 0x0800 then the
> > +                 *     packet is an IPv4 packet).
> > +                 *
> > +                 * Thus, if OVS_KEY_ATTR_ETHERNET is present, even if it is
> > +                 * all-wildcarded, it is important to print it.
> > +                 *
> > +                 * On the other hand, the userspace datapath supports
> > +                 * OVS_KEY_ATTR_PACKET_TYPE and uses it to indicate the packet
> > +                 * type.  Thus, if OVS_KEY_ATTR_PACKET_TYPE is present, we need
> > +                 * not print an all-wildcarded OVS_KEY_ATTR_ETHERNET. */
> > +                if (!first_field) {
> > +                    ds_put_char(ds, ',');
> > +                }
> > +                ds_put_cstr(ds, "eth()");
> >              }
> >              ofpbuf_clear(&ofp);
> >          }
> > --
> > 2.16.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev