[ovs-dev] [PATCH v8] Extend OVS IPFIX exporter to export tunnel headers

[Wenyu Zhang]

-----Original Message-----
From: Pravin Shelar [mailto:pshelar at nicira.com] 
Sent: Thursday, August 14, 2014 8:37 AM
To: Wenyu Zhang
Cc: Ben Pfaff; Jesse Gross; Romain Lenglet; dev at openvswitch.org
Subject: Re: [PATCH v8] Extend OVS IPFIX exporter to export tunnel headers

On Wed, Aug 13, 2014 at 5:25 PM, Wenyu Zhang <wenyuz at vmware.com> wrote:
>>> Signed-off-by: Wenyu Zhang <wenyuz at vmware.com>
>>> Acked-by: Romain Lenglet <rlenglet at vmware.com>
>>
>> This patch renumbers OVS_TUNNEL_KEY_ATTR_OAM and 
>> OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS.  Is that really OK?
>>
> It is not ok.
> There are another fixes to kernel part which I was going to do before pushing this patch to master.
>
> Thanks,
> Pravin.
>
> Wenyu: I can move the new items after OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS if it is not OK.
> And I am wondering why it is not OK?  Is there any case that the renumbering will cause issue?

If we renumber kernel interface enum, OVS does not work if there is mismatch userspace and ovs kernel module versions. So to maintain compatibility we add new enum at the end.

Wenyu: Thanks for your answering. It sounds reasonable. 
But  when userpace and kernel module versions mismatches, OVS may not work well even though the old kernel interface enum not changed due to the userland or kernel module data path may be changed.  
Keeping the existent kernel interface enum unchanged may reduce the possibility that OVS doesn't work when the userpace and kernel module versions mismatches, but can not avoid the possibility totally. Is that true?

> Thanks a lot.
>
>> @@ -338,10 +345,12 @@ enum ovs_tunnel_key_attr {
>>         OVS_TUNNEL_KEY_ATTR_IPV4_DST,           /* be32 dst IP address. */
>>         OVS_TUNNEL_KEY_ATTR_TOS,                /* u8 Tunnel IP ToS. */
>>         OVS_TUNNEL_KEY_ATTR_TTL,                /* u8 Tunnel IP TTL. */
>>         OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT,      /* No argument, set DF. */
>>         OVS_TUNNEL_KEY_ATTR_CSUM,               /* No argument. CSUM packet. */
>> +       OVS_TUNNEL_KEY_ATTR_TP_SRC,             /* be16 src Transport Port. */
>> +       OVS_TUNNEL_KEY_ATTR_TP_DST,             /* be16 dst Transport Port. */
>>         OVS_TUNNEL_KEY_ATTR_OAM,                /* No argument, OAM frame. */
>>         OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,        /* Array of Geneve options */
>>         __OVS_TUNNEL_KEY_ATTR_MAX
>>  };
>>  #define OVS_TUNNEL_KEY_ATTR_MAX (__OVS_TUNNEL_KEY_ATTR_MAX - 1)
>>
>> I'm continuing to look at it.