[ovs-dev] [PATCH v3 4/9] datapath-windows: Add support for Conntrack IPCTNL_MSG_CT_DELETE cmd in Datapath.c

Nithin Raju nithin at vmware.com
Mon Jun 27 22:18:43 UTC 2016


Looks good but for a couple of suggestions.

>/* Windows kernel datapath extensions to the standard datapath interface.
>*/
> 
> /* Version number of the datapath interface extensions. */
>@@ -65,6 +68,7 @@
> #define OVS_WIN_NL_VPORT_FAMILY_ID           (NLMSG_MIN_TYPE + 4)
> #define OVS_WIN_NL_FLOW_FAMILY_ID            (NLMSG_MIN_TYPE + 5)
> #define OVS_WIN_NL_NETDEV_FAMILY_ID          (NLMSG_MIN_TYPE + 6)
>+#define OVS_WIN_NL_CT_FAMILY_ID              (NLMSG_MIN_TYPE + 7)
> 
> #define OVS_WIN_NL_INVALID_MCGRP_ID          0
> #define OVS_WIN_NL_MCGRP_START_ID            100
>@@ -156,4 +160,17 @@ enum ovs_win_netdev_attr {
> typedef struct ovs_dp_stats OVS_DP_STATS;
> typedef enum ovs_vport_type OVS_VPORT_TYPE;
> 
>+/* Conntrack Netlink */
>+#define NFNL_TYPE_CT_GET (NFNL_SUBSYS_CTNETLINK << 8 | IPCTNL_MSG_CT_GET)
>+#define NFNL_TYPE_CT_DEL (NFNL_SUBSYS_CTNETLINK << 8 |
>IPCTNL_MSG_CT_DELETE)
>+#define NFNL_SUBSYSTEM_TYPE(nlmsgType) (nlmsgType >> 8)
>+#define NFNL_CT_CMD(nlmsgType) (nlmsgType & 0xff)
>+#define IS_NFNL_CMD(nlmsgType) ((nlmsgType == NFNL_TYPE_CT_GET) ||
>(nlmsgType == NFNL_TYPE_CT_DEL))
>+#define OVS_NL_CT_ATTR_MAX (IPCTNL_MSG_MAX - 1)

This will probably change with the discussion we are having about where
the netlink defines should go to. If we decide to go with an approach of
OvsDpInterfaceCtExt.h, it would be nice to put these in enums, with
comments around them.

The only thing we need to be careful about adding code in any of the
OvsDpInterface*h. is that we¹ll be stuck with it for the foreseeable
future. We¹ll always have to make future code adhere to this interface.

> /* Netlink netdev family. */
> NETLINK_CMD nlNetdevFamilyCmdOps[] = {
>     { .cmd = OVS_WIN_NETDEV_CMD_GET,
>@@ -885,6 +904,9 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> 
>     ASSERT(ovsMsg);
>     switch (ovsMsg->nlMsg.nlmsgType) {
>+    case NFNL_TYPE_CT_DEL:
>+        nlFamilyOps = &nlCtFamilyOps;
>+        break;
>     case OVS_WIN_NL_CTRL_FAMILY_ID:
>         nlFamilyOps = &nlControlFamilyOps;
>         break;
>@@ -961,6 +983,30 @@ ValidateNetlinkCmd(UINT32 devOp,
>         goto done;
>     }
> 
>+    /*
>+        Verify if the Netlink message is part of Netfilter Netlink
>+        This is currently used by Conntrack
>+    */

minor: comment styling should be:
/*
 *
 *
 */
>@@ -1022,14 +1068,29 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
>     NTSTATUS status = STATUS_INVALID_PARAMETER;
>     UINT16 i;
> 
>-    for (i = 0; i < nlFamilyOps->opsCount; i++) {
>-        if (nlFamilyOps->cmds[i].cmd ==
>usrParamsCtx->ovsMsg->genlMsg.cmd) {
>-            NetlinkCmdHandler *handler = nlFamilyOps->cmds[i].handler;
>-            ASSERT(handler);
>-            if (handler) {
>-                status = handler(usrParamsCtx, replyLen);
>+    if (IS_NFNL_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType)) {
>+        /* If nlMsg is of type Netfilter-Netlink parse the Cmd
>accordingly */
>+        UINT8 cmd = NFNL_CT_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType);
>+        for (i = 0; i < nlFamilyOps->opsCount; i++) {
>+            if (nlFamilyOps->cmds[i].cmd == cmd) {
>+                NetlinkCmdHandler *handler =
>nlFamilyOps->cmds[i].handler;
>+                ASSERT(handler);
>+                if (handler) {
>+                    status = handler(usrParamsCtx, replyLen);
>+                }
>+                break;
>+            }
>+        }
>+    } else {
>+        for (i = 0; i < nlFamilyOps->opsCount; i++) {
>+            if (nlFamilyOps->cmds[i].cmd ==
>usrParamsCtx->ovsMsg->genlMsg.cmd) {
>+                NetlinkCmdHandler *handler =
>nlFamilyOps->cmds[i].handler;
>+                ASSERT(handler);
>+                if (handler) {
>+                    status = handler(usrParamsCtx, replyLen);
>+                }
>+                break;
>             }
>-            break;
>         }

Suggestion:
This code could be refactored:
if (NF) {
  cmd = xx;
} else {
  cmd = usrParamsCtx->ovsMsg->genlMsg.cmd;
}

for (i = 0; i < nlFamilyOps->opsCount; i++) {
    if (nlFamilyOps->cmds[i].cmd == usrParamsCtx->ovsMsg->genlMsg.cmd) {
[Š]





More information about the dev mailing list