[ovs-dev] [PATCH 4/4] datapath-windows: add support for GET_DP command to dump datpaths (Nithin Raju)

Nithin Raju nithin at vmware.com
Fri Aug 29 08:06:50 UTC 2014


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

> Hello Nithin,
> 
>> /* Netlink datapath family. */
>> NETLINK_CMD nlDatapathFamilyCmdOps[] = {
>>    { OVS_DP_CMD_GET, OvsGetDpCmdHandler,
>>      OVS_WRITE_DEV_OP | OVS_READ_DEV_OP, FALSE }
>> };
> 
> Could you make the NETLINK_CMD variable initialization in the same style as the NETLINK_FAMILY variable initializations?
> It would also make it a bit more obvious as what "FALSE" is above.

Done.

> Regarding OvsGetDpCmdHandler:
> The function is quite big.
> Could you split it something like,
> "
> if (instance->dumpState.ovsMsg == NULL) {
>  OvsDpDumpStart(params)
> } else {
>  OvsDpDumpNextparams)
> }
> "
> 
> on the "dump start" part:
> it might also be the case that all dump operations (dp, vport, etc.) will share this exact code portion.

Good point. I wasn't thinking so much forward :) I added a new function OvsSetupDumpStart(), which as you point out is not specific to Dp Dump.

>>       msgOut->nlMsg.nlmsgType = OVS_WIN_NL_DATAPATH_FAMILY_ID;
>>       msgOut->nlMsg.nlmsgFlags = 0;  /* XXX: ? */
>>       msgOut->nlMsg.nlmsgSeq = msgIn->nlMsg.nlmsgSeq;
>>       msgOut->nlMsg.nlmsgPid = msgIn->nlMsg.nlmsgPid;
>>       msgOut->nlMsg.nlmsgLen = sizeof *msgOut;
>> 
>>       msgOut->genlMsg.cmd = OVS_DP_CMD_GET;
>>       msgOut->genlMsg.version = nlDatapathFamilyOps.version;
>>       msgOut->genlMsg.reserved = 0;
> 
> This is a pattern that we will see everywhere in the netlink commands. We could add a function now "OvsCreateOutMsgFromInMsg" or we could do it later.
> 
> btw, the 'netlink set api' is the last that remains of the netlink infrastructure, right?

Yes, I am hoping that once the netlink set API is in place, we'll refactor this.

Thanks so much for the review.

thanks,
Nithin


More information about the dev mailing list