I applied the combined result but after I did that I noticed that it
introduces another bug: a ToS of 0 must be explicitly specified, there
is no default value for this field. Therefore, when we serialize it
back we should always include it.
Please be more careful when changing code like this, particularly
protocol code which tends to be large and hard to review. There have
been a number of subtle bugs that have been introduced because
behavior was silently changed in a larger patchset without being
analyzed or called out.
On Tue, Jul 9, 2013 at 4:01 PM, Andy Zhou <az...@nicira.com> wrote:
> Thanks. An incremental patch is attached that should fix both issues raised.
>
>
> On Tue, Jul 9, 2013 at 1:59 PM, Jesse Gross <je...@nicira.com> wrote:
>>
>> On Tue, Jul 9, 2013 at 12:25 AM, Andy Zhou <az...@nicira.com> wrote:
>> > diff --git a/datapath/flow.c b/datapath/flow.c
>> > index adb918f..a756a15 100644
>> > --- a/datapath/flow.c
>> > +++ b/datapath/flow.c
>> > @@ -1686,16 +1688,15 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
>> > *swkey,
>> [...]
>> > - if (swkey->phy.in_port != DP_MAX_PORTS) {
>> > - /* Exact match upper 16 bits. */
>> > + {
>> > u16 upper_u16;
>> > upper_u16 = (swkey == output) ? 0 : 0xffff;
>> >
>>
>> DP_MAX_PORTS is an internal kernel value that has no meaning to
>> userspace so we shouldn't leak it. I think we need to do something
>> similar to 802.2 where the lack of a attribute implies a particular
>> value.
>>
>> I also see another problem here: on flow translation from userspace we
>> don't set DP_MAX_PORTS when there is no attribute. Instead, it just
>> remains as zero, which is the internal port.
>>
>> > @@ -1728,12 +1729,20 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
>> > *swkey,
>> > } else
>> > encap = NULL;
>> >
>> > - if ((swkey == output) && (swkey->eth.type ==
>> > htons(ETH_P_802_2)))
>> > + if (swkey->eth.type == htons(ETH_P_802_2)) {
>> > + /*
>> > + * Ethertype 802.2 is represented in the netlink with
>> > omitted
>> > + * OVS_KEY_ATTR_ETHERTYPE in the flow key attribute, and
>> > + * 0xffff in the mask attribute.
>> > + */
>> > + if (swkey != output)
>> > + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>> > htons(0xffff)))
>> > + goto nla_put_failure;
>> > goto unencap;
>> > + }
>>
>> I'm not sure that this is right - it implies that we must always have
>> an exact match on the EtherType for 802.2 packets but I think it
>> really is that we must either have exact match or a complete wildcard
>> on it. For example, it should be possible to create a flow where the
>> original packet is 802.2 but you want to have a flow that just matches
>> on the MAC addresses.
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
