[ovs-dev] [PATCH 1/2] datapath-windows: fix bug in NlBufCopyAtTailUninit

[Ankur Sharma]

Hi Nithin,

Thanks a lot for making the changes.

One minor comment:
 if ((NlBufCopyAtTail(nlBuf, NULL, len)) == FALSE) {
        return NULL;  <-- we can do ret=NULL so that we have only one return statement in this function.
    }

Regarding naming:
I have no strong preference. I kept CopyAt in the function name to be consistent with other functions. But if leads to confusion then we can move to the naming which sam has recommended. But yes, it need not come in this series. We can take care of it in a separate patch.

Acked-by: Ankur Sharma <ankursharma at vmware.com>

Thanks.

Regards,
Ankur
________________________________________
From: dev <dev-bounces at openvswitch.org> on behalf of Nithin Raju <nithin at vmware.com>
Sent: Monday, September 15, 2014 12:26 AM
To: Samuel Ghinet
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH 1/2] datapath-windows: fix bug in NlBufCopyAtTailUninit

On Sep 14, 2014, at 8:18 PM, Samuel Ghinet <sghinet at cloudbasesolutions.com>
 wrote:

> At first on reading this code, I was wondering "why?"
> Now I think I understand: the function zeroes memory, and must return the ptr to 'next' item to be filled (which is called "tail"). Am I correct?

Yes, that is the bug. The pointer returned is the same pointer at where memory was zeroed.

> Also, I think it would be nice if you could rename NlBufCopyAtTailUninit -> NlBufZeroAtTailUninit or smth similar, because it does not actually do a copy, and the name makes it a bit confusing.
> But perhaps this would also work as a separate patch.

Sure, I'll talk to Ankur about this since he designed and wrote this code. He might have some thoughts too.

Thanks for the review. You can ack the v2 patch, which I'll send out.

Thanks,
-- Nithin

_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=8l2QScg0CJXimjkl0edvkTdBNBKzmqyDjnxQXpU1lAI%3D%0A&s=51916c5716f8a68bc7a99ccf8df1733f28570489b3944675df508d87c5c82955