[ovs-dev] [PATCH] datapath-windows: Solved BSOD when handling flows

[Sorin Vinturis]

Nithin,

Thanks for your comment. I agree with what you said. This patch removes a wrong call to FreeFlow() function. Do you see any issue with this patch?

-Sorin

-----Original Message-----
From: Nithin Raju [mailto:nithin at vmware.com] 
Sent: Monday, 3 August, 2015 18:27
To: Sorin Vinturis
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Solved BSOD when handling flows

> On Jul 1, 2015, at 10:28 AM, Sorin Vinturis <svinturis at cloudbasesolutions.com> wrote:
> 
> OvsPrepareFlow() returns an error only when the new flow allocation 
> fails. In this case HandleFlowPut() should return error without trying 
> to free the flow, thus avoiding the BSOD.
> 
> Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
> Reported-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
> Reported-at: 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvs
> witch_ovs-2Dissues_issues_91&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-
> YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=1fAwiEJ0
> trEqEbnXCwc2WN_OdJ9ANd0-fuucgcbW9Bs&s=v7pTsmCFx-lu-RBhxiVv5cbM5jUrnSC6
> P6sP6dp-PRw&e=
> ---
> This patch should be applied both on master and branch 2.4.
> ---
> datapath-windows/ovsext/Flow.c | 1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/datapath-windows/ovsext/Flow.c 
> b/datapath-windows/ovsext/Flow.c index 6fa10a3..b93f475 100644
> --- a/datapath-windows/ovsext/Flow.c
> +++ b/datapath-windows/ovsext/Flow.c
> @@ -2163,7 +2163,6 @@ HandleFlowPut(OvsFlowPut *put,
> 
>         status = OvsPrepareFlow(&KernelFlow, put, hash);
>         if (status != STATUS_SUCCESS) {
> -            FreeFlow(KernelFlow);
>             return STATUS_UNSUCCESSFUL;
>         }

I’m looking at OvsPrepareFlow(), and the only case where we the function can fail is if there’s a memory allocation failure at which point ‘KernelFlow’ would end up being NULL, so, FreeFlow() will actually ASSERT().

Moreover, I think it would be wrong for the caller to free up an object that is allocated within OvsPrepareFlow(). I believe that if the function OvsPrepareFlow() fails, it should take responsibility for cleaning the objects that were allocated in the context of the function, and not expect that caller to do so.

thanks,
-- Nithin