[ovs-dev] [PATCH] ovs-hyperv: make kernel return values netlink socket like

Nithin Raju nithin at vmware.com
Tue Apr 28 14:38:51 UTC 2015


hi Sorin,
Thanks so much for the review.

Let me address your comments and then send out the updated patch. If I missed anything, pls. let me know.

>     UINT8 version;
> -    UINT8 pad;
> +    UINT8 pad1;
>     UINT16 maxAttr;
> +    UINT16 pad2;
> SV: Do we need the new 'pad' members, 'pad1' & 'pad2'? I didn't saw them referenced in any part of the code.

I used them to explicitly add padding in the structure to avoid holes.

>     NETLINK_CMD *cmds;          /* Array of netlink commands and handlers. */
>     UINT16 opsCount;
> } NETLINK_FAMILY, *PNETLINK_FAMILY;
> @@ -143,12 +144,12 @@ NETLINK_CMD nlControlFamilyCmdOps[] = {
>     },
>     { .cmd = OVS_CTRL_CMD_EVENT_NOTIFY,
>       .handler = OvsReadEventCmdHandler,
> -      .supportedDevOp = OVS_READ_EVENT_DEV_OP,
> +      .supportedDevOp = OVS_READ_DEV_OP,
>       .validateDpIndex = FALSE,
>     },
>     { .cmd = OVS_CTRL_CMD_READ_NOTIFY,
>       .handler = OvsReadPacketCmdHandler,
> -      .supportedDevOp = OVS_READ_PACKET_DEV_OP,
> SV: The two operations, 'OVS_READ_EVENT_DEV_OP' and 'OVS_READ_PACKET_DEV_OP' are not used anymore. Shouldn't we remove them from the header as well?

I’ve removed it in the header file later.

> /*
>  * --------------------------------------------------------------------------
> - * Function to invoke the netlink command handler.
> + * Function to invoke the netlink command handler. The function also 
> + massages
> + * the return value of the handler function to construct a 'NL_ERROR' 
> + message
> SV: Please rephrase the function description above - 'massage' is not appropriate.

Sure. I’ll do that.

-- Nithin


More information about the dev mailing list