[ovs-dev] [PATCH 1/4] datapath-windows: Data structures and functions for dump state (Nithin Raju)

Nithin Raju nithin at vmware.com
Fri Aug 29 07:44:11 UTC 2014


On Aug 28, 2014, at 6:40 PM, Samuel Ghinet <sghinet at cloudbasesolutions.com>
 wrote:

> Hello Nithin,
> 
> I have a few notes regarding the patch:
> 
>>    struct {
>>        POVS_MESSAGE ovsMsg;    /* OVS message passed during dump start. */
>>        UINT32 index[8];        /* markers to continue dump from. One or more
>>                                 * of them may be used. */
>>    } dumpState;                /* data to support dump commands. */
> 
> 1. It might be useful if you would add a comment to index[8] to express how this index field is going to be used in dump operations.
> I am sure the way the indexes are used in the dump operations would be quite the same in all the project.

Done. I put 8 markers, since Linux was also doing the same. I've set this to 2 markers now.

> 2. Do we need 8 markers? If we may need only index[0], perhaps we should keep only one index.
> If there will be a part in code where two indexes will be used, perhaps we should add then, a new variable, with a more specific name.
> Specific names for how the index(es) is / are actually used might be clearer than using index[0], index[1], etc.

Sure, whoever ever implements flow dump which is probably the command that uses multiple markers can update it. For now, I want to keep this generic.

> btw, pershaps one day we should rename the OVS_OPEN_INSTANCE to something more specific, such as an OVS_FILE_HANDLE_CONTEXT.

Sure, I am all for making code clearer :) I have lot of examples in mind too :)

> function InitUserDumpState:
>> instance->dumpState.ovsMsg = (POVS_MESSAGE) OvsAllocateMemory(
>>       sizeof *instance->dumpState.ovsMsg);
> 
> I think it would be cleaner (and shorter) if you could do instead:
> instance->dumpState.ovsMsg = (POVS_MESSAGE) OvsAllocateMemory(
>        sizeof OVS_MESSAGE);
> 
> "OVS_MESSAGE" is much shorter to read than "*instance->dumpState.ovsMsg"

Done.

Thanks for the review. I'll send out a patch.

thanks,
Nithin


More information about the dev mailing list