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