Sorry, I meant to say TTL instead of ToS.
On Tue, Jul 9, 2013 at 4:43 PM, Jesse Gross <je...@nicira.com> wrote:
> 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
