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

Jarno Rajahalme jrajahalme at nicira.com
Wed Apr 2 21:51:10 UTC 2014


I’ll also change all the “getter” functions to be spelled without the “get”, as done by Pravin for his patch earlier today on ofpbufs.

Unless there are other comments, I’ll not send a new patch for review, as this is not affecting functionality in any way.

  Jarno

On Apr 2, 2014, at 11:33 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:

> 
> 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
> 




More information about the dev mailing list