On Mon, Mar 06, 2017 at 12:37:11AM +0000, Jan Scheurich wrote:
> > I see that match_format() prints packet_type as a hex number. I doubt
> > that's the best for human consumption. At the very least, it seems like
> > it's better printed as a pair of numbers, and probably it would be more
> > friendly to say what packet type it is.
>
> I agree that a pair of numbers would be more user friendly. Would you
> prefer packet_type=(1,0x800) or packet_type={1,0x800} or some other
> notation.
>
> For the most common Ethernet packet_type (0,0) we would print
> packet_type=ethernet.
>
> Symbolic packet_type representation for namespace 1 could easily lead to
> confusion with the legacy match short-hands such as "ip", "ipv6", "arp",
> "udp", etc. I believe these will need to remain untouched, e.g. packet_type=0
> && dl_type=0x800 && nw_proto=17 for "udp", or we would break a lot of legacy.
>
> Here we should really agree on something up front. Changing the
> notation is very painful due to the large number of unit test cases
> that have to be adapted.
I think that packet_type=(0,0x800) is the choice closest to the existing
formats.
I wonder whether we need to print a packet_type for Ethernet at all. We
could decide to print nothing for a match on Ethernet, and instead print
"packet_type=*" if the packet type is wildcarded. I suspect that this
would result in no change in the common case.
> > Can we fix whatever problem this is instead of working around it?
> >
> > + /* Wildcard ethernet addresses if the original packet type was not
> > + * Ethernet.
> > + * XXX: This is a work-around. ofproto shouldn't unwildcard the
> > Ethernet
> > + * addresses at all. */
> > + if (ctx->xin->upcall_flow->packet_type != PT_ETH) {
> > + memset(&ctx->wc->masks.dl_dst, 0, ETH_ADDR_LEN);
> > + memset(&ctx->wc->masks.dl_src, 0, ETH_ADDR_LEN);
> > + }
>
> I had a discussion with Jarno about this
> (https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327510.html)
> and agreed that this is needed unless ofproto-dpif-xlate maintains a
> separate mask to keep track of the field bits that have been set
> independently from the bits that have been matched. That might well be
> worth doing to get wider, more generic megaflows, but it should be a
> separate patch series. I can remove the XXX: comment.
OK.
> > and this?
> >
> > + /* XXX: ONLY FOR NON-PTAP BRIDGE! */
> > + if (flow->packet_type != PT_ETH && in_port && in_port->is_layer3 &&
> > + ctx.table_id == 0) {
> > + /* Add dummy Ethernet header to non-L2 packet if it's coming from a
> > + * L3 port. So all packets will be L2 packets for lookup.
> > + * The dl_type has already been set from the packet_type. */
> > + flow->packet_type = PT_ETH;
> > + memset(&flow->dl_src, 0, ETH_ADDR_LEN);
> > + memset(&flow->dl_dst, 0, ETH_ADDR_LEN);
> > + }
>
> This is not a work-around. Again the comment was more for us to
> remember what needs revising when implementing PTAP bridges. We can
> remove it.
OK, thanks.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
