[ovs-dev] [RFC PATCH 4/4] lib/ofpbuf: Compact

[Jarno Rajahalme]

On Apr 1, 2014, at 1:28 PM, Lori Jakab <lojakab at cisco.com> wrote:

> On 4/1/14, 5:43 PM, Jarno Rajahalme wrote:
>> 
>> Sent from my iPhone
>> 
>>> On Apr 1, 2014, at 7:27 AM, Ben Pfaff <blp at nicira.com> wrote:
>>> 
>>>> On Tue, Apr 01, 2014 at 03:14:16PM +0300, Lori Jakab wrote:
>>> Can we do the following?
>>>    3) Replace 'l2' by 'l2_ofs' also?
>> I'll look into this today. However, my earlier analysis was that this would complicate things when the offset basis ('data') is changing a lot (e.g., when pulling data from ofpbuf). Maybe the cleanest solution would be to have a pointer to the start of packet and an additional l2 offset.
> 
> Yes, that's the most elegant solution IMHO.

I just sent out a patch that renames ‘l2’ as ‘frame’, but does not add an l2 offset, as it is not needed. We already have a condition, when the ‘frame’ is set, but there is no Ethernet header, which happens if the frame is too short to hold and ethernet frame. We have not checked for this in past as it has not happened in practice, but I added some checks for this. The new ofpbuf_get_l2() now returns NULL if the l3 offset is not set.

When you add support to non-Ethernet packets, you should add a condition “l3_ofs != 0” to the ofpbuf_get_l2(), so that it returns NULL if l3_ofs is zero.

Do you have plans to support MPLS packets without Ethernet header? If yes, you will also need to take l2_5 offset into consideration.

> 
>> However, as 0 is a valid offset value, having a separate l2 offset seems redundant as l3_ofs 0 would be an unambiguous way of encoding the absence of the l2 header.
> 
> I think having 'l2' set to a value other then NULL for layer 3 packets can be confusing, even if l3_ofs is set to 0, but I don't mind implementing L3 support that way.
> 

Maybe this is less conferring after renaming ‘l2’ as ‘frame’. Also, the new ofpbuf_get_l2() is now used only when we explicitly want the address of the Ethernet frame. Other uses of ofpbuf (see the new comment in the patch) use ‘frame’ directly.

> I like Ben's proposal 3) better than 2), but I understand the concerns about the basis changing a lot.
> 

I actually tried to use ‘data’ as the basis, but is turned out to be impossible with out current practice of setting the ‘l2’ and ‘l3’ to the OpenFlow header and then pulling ‘data’ past it; the offsets would have become negative and each _pull and _push operation would have needed to update all the offsets.

  Jarno

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140402/a2f6c755/attachment-0005.html>