On Aug 28, 2014, at 6:40 PM, Samuel Ghinet <[email protected]>
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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
