[ovs-dev] [PATCH] ofpbuf: Abstract 'l2' pointer and document usage conventions.

[Jarno Rajahalme]

On Apr 2, 2014, at 11:23 AM, Lori Jakab <lojakab at cisco.com> wrote:

> On 4/2/14, 8:59 PM, Jarno Rajahalme wrote:
> 
> [...]
> 
>> diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
>> index 8d1cb11..fa8a3f0 100644
>> --- a/lib/ofpbuf.h
>> +++ b/lib/ofpbuf.h
>> @@ -36,26 +36,46 @@ enum OVS_PACKED_ENUM ofpbuf_source {
>>  };
>>    /* Buffer for holding arbitrary data.  An ofpbuf is automatically reallocated
>> - * as necessary if it grows too large for the available memory. */
>> + * as necessary if it grows too large for the available memory.
>> + *
>> + * 'frame' and offset conventions:
>> + *
>> + * Network frames (aka "packets"): 'frame' MUST be set to the start of the
>> + *    packet, layer offsets MAY be set as appropriate for the packet.
>> + *    Additionally, we assume in many places that the 'frame' and 'data' are
>> + *    the same for packets.
>> + *
>> + * OpenFlow messages: 'frame' points to the start of the OpenFlow
>> + *    header, while 'l3_ofs' is the length of the OpenFlow header.
>> + *    When parsing, the 'data' will move past these, as data is being
>> + *    pulled from the OpenFlow message.
>> + *
>> + * Actions: When encoding OVS action lists, the 'frame' is used
>> + *    as a pointer to the beginning of the current action (see ofpact_put()).
>> + *
>> + * rconn: Reuses 'frame' as a private pointer while queuing.
>> + */
>>  struct ofpbuf {
>>      void *base;                 /* First byte of allocated space. */
>>      uint32_t allocated;         /* Number of bytes allocated. */
>>      uint32_t size;              /* Number of bytes in use. */
>>      void *data;                 /* First byte actually in use. */
>>  -    void *l2;                   /* Link-level header. */
>> -    uint16_t l2_5_ofs;          /* MPLS label stack offset from l2, or
>> +    void *frame;                /* Packet frame start, or NULL. */
>> +    uint16_t l2_5_ofs;          /* MPLS label stack offset from 'packet', or
>>                                   * UINT16_MAX */
>> -    uint16_t l3_ofs;            /* Network-level header offset from l2, or
>> -                                 * UINT16_MAX. */
>> -    uint16_t l4_ofs;            /* Transport-level header offset from l2, or
>> -                                   UINT16_MAX. */
>> +    uint16_t l3_ofs;            /* Network-level header offset from 'packet',
>> +                                   or UINT16_MAX. */
>> +    uint16_t l4_ofs;            /* Transport-level header offset from 'packet',
>> +                                   or UINT16_MAX. */
> 
> Shouldn't you say 'frame' in the comments instead of 'packet' (using quotes usually refers to struct members) ?

Yes I should. I’ll fix this before pushing to master.

Thanks,

  Jarno