[ovs-dev] [PATCH] datapath-windows: OVS_PACKET_ATTR_PROBE

[Alin Serdean]

Sorry was relocating home.

Thanks for raising the question, it made me realize another problem :)

To answer your question directly: no this patch just ignores the attribute completely it does not do any additional operations.

I introduced the attribute because it was failing at https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/User.c#L341, and to be more direct because the optional parameter was zero but apparently this is just the symptom.

Due to your question I relooked over the code and found the actual problem: when we are calling NlAttrParse we are calling it with array size of the __OVS_PACKET_ATTR_MAX(the one defined in openvswitch.h) when we should call it with the array size of the policy itself.

I will look over the windows datapath code to see if we slipped in that mistake in other parts as well and will try to send the patches asap.

Regarding the patch it is semi dead weight because it just introduces the attribute but no real logic is behind (it will work with and without it).

Alin.

-----Mesaj original-----
De la: Ben Pfaff [mailto:blp at nicira.com] 
Trimis: Thursday, July 2, 2015 2:36 AM
Către: Eitan Eliahu
Cc: Alin Serdean; dev at openvswitch.org
Subiect: Re: [ovs-dev] [PATCH] datapath-windows: OVS_PACKET_ATTR_PROBE

Alin?

On Wed, Jul 01, 2015 at 11:04:57PM +0000, Eitan Eliahu wrote:
> Hi Ben,
> In general it doesn't seem that if the datapath does not support an attribute it would fail. (In other words the parsing validation code will not fail if an attribute is not included in the policy array).
> I am not sure if the addition of this specific attribute to the policy masks an issue or not.
> Perhaps Alin can tell.
> Thanks,
> Eitan
> 
> -----Original Message-----
> From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Wednesday, July 01, 2015 2:53 PM
> To: Alin Serdean
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] datapath-windows: OVS_PACKET_ATTR_PROBE
> 
> On Wed, Jul 01, 2015 at 06:57:50PM +0000, Alin Serdean wrote:
> > Since commit:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_open
> > vs 
> > witch_ovs_commit_2e460098bff351b9fddcb917447caa3b97a35d86&d=BQIGaQ&c
> > =S 
> > qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ
> > 4e 
> > 3geGYp56lkcH-5cLCY&m=wSN1s5IDTOoE-dDGQWtmN87LEkQnYq71hKWcn7d3SJk&s=8
> > _A Va78P_KXRWsNoVVtFSjNbBrJjyie8OjjjH4U953U&e=
> > a new packet attribute was introduced.
> > 
> > This patch adds OVS_PACKET_ATTR_PROBE to nlPktExecPolicy in 
> > datapath-windows and ignores it for the moment to maintain binary compatibility.
> > 
> > Signed-off-by: Alin Gabriel Serdean 
> > <aserdean at cloudbasesolutions.com>
> > Acked-by: Eitan Eliahu <eliahue at vmware.com>
> > ---
> > This patch should be applied on master and branch-2.4
> > v2: add acked-by
> 
> This patch makes me worry a bit.  The Linux kernel and OVS userspace implementations of Netlink ignore attributes that they do not recognize, which allows for some kinds of compatibility.  To me, this patch implies that the Windows datapath reports an error when it sees an unrecognized attribute.  Is that correct?  If so, then that should be changed, because otherwise it will cause compatibility problems in the future.
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_ma
> ilman_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-
> uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=wSN1s5IDTOoE-dDGQW
> tmN87LEkQnYq71hKWcn7d3SJk&s=TZgQzcgvt8NDu-lf_W0nI9BkcWxoExr-9T2PSSonAb
> k&e=