diff mbox

[ovs-dev,9/9] datapath-windows: Add support for Conntrack IPCTNL_MSG_CT_GET cmd in Datapath.c

Message ID 1466472188-3288-10-git-send-email-vsairam@vmware.com
State Superseded
Headers show

Commit Message

Sairam Venugopal June 21, 2016, 1:23 a.m. UTC 
This will be used by userspace for dumping conntrack entries - "ovs-dpctl
dump-conntrack".

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
---
 datapath-windows/ovsext/Datapath.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Paul Boca June 23, 2016, 7:50 p.m. UTC | #1 
Hi Sai!

Please see my comments inline.
Besides that it looks good to me.

Thanks,
Paul

> -----Original Message-----

> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Sairam

> Venugopal

> Sent: Tuesday, June 21, 2016 4:23 AM

> To: dev@openvswitch.org

> Subject: [ovs-dev] [PATCH 9/9] datapath-windows: Add support for

> Conntrack IPCTNL_MSG_CT_GET cmd in Datapath.c

> 

> This will be used by userspace for dumping conntrack entries - "ovs-dpctl

> dump-conntrack".

> 

> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>

> ---

>  datapath-windows/ovsext/Datapath.c | 12 ++++++++++--

>  1 file changed, 10 insertions(+), 2 deletions(-)

> 

> diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-

> windows/ovsext/Datapath.c

> index 7cc8390..5cc0614 100644

> --- a/datapath-windows/ovsext/Datapath.c

> +++ b/datapath-windows/ovsext/Datapath.c

> @@ -104,7 +104,8 @@ NetlinkCmdHandler        OvsGetNetdevCmdHandler,

>                           OvsPendPacketCmdHandler,

>                           OvsSubscribePacketCmdHandler,

>                           OvsReadPacketCmdHandler,

> -                         OvsCtDeleteCmdHandler;

> +                         OvsCtDeleteCmdHandler,

> +                         OvsCtDumpCmdHandler;

> 

>  static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT

> usrParamsCtx,

>                                         UINT32 *replyLen);

> @@ -288,7 +289,13 @@ NETLINK_CMD nlCtFamilyCmdOps[] = {

>      { .cmd              = IPCTNL_MSG_CT_DELETE,

>        .handler          = OvsCtDeleteCmdHandler,

>        .supportedDevOp   = OVS_TRANSACTION_DEV_OP,

> -      .validateDpIndex  = TRUE

> +      .validateDpIndex  = FALSE

> +    },

> +    { .cmd              = IPCTNL_MSG_CT_GET,

> +      .handler          = OvsCtDumpCmdHandler,

> +      .supportedDevOp   = OVS_TRANSACTION_DEV_OP |

> +                          OVS_WRITE_DEV_OP | OVS_READ_DEV_OP,

> +      .validateDpIndex  = FALSE

>      }

>  };

[Paul Boca] Now you have 2 NETLINK_CMDs, maybe it would be nice to declare
a NETLINK_FAMILY which points to nlCtFamilyCmdOps. It is functional but it
doesn't mentain the style used here.

> 

> @@ -904,6 +911,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,

> 

>      ASSERT(ovsMsg);

>      switch (ovsMsg->nlMsg.nlmsgType) {

> +    case NFNL_TYPE_CT_GET:

[Paul Boca] If you declare a new NETLINK_FAMILY then you will have here only 
one case with OVS_WIN_..._FAMILY_ID

>      case NFNL_TYPE_CT_DEL:

>          nlFamilyOps = &nlCtFamilyOps;

>          break;

> --

> 2.5.0.windows.1

> 

> _______________________________________________

> dev mailing list

> dev@openvswitch.org

> http://openvswitch.org/mailman/listinfo/dev
Sairam Venugopal June 23, 2016, 8:41 p.m. UTC | #2 
Hi Paul,

Thanks for the review and comments. I will incorporate them in v2.

Regarding the current patch -

I do have a NETLINK_FAMILY defined:
/* Netlink Ct family. */
NETLINK_CMD nlCtFamilyCmdOps[] = {
    { .cmd              = IPCTNL_MSG_CT_DELETE,
      .handler          = OvsCtDeleteCmdHandler,
      .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
      .validateDpIndex  = FALSE
    },
    { .cmd              = IPCTNL_MSG_CT_GET,
      .handler          = OvsCtDumpCmdHandler,
      .supportedDevOp   = OVS_TRANSACTION_DEV_OP |
                          OVS_WRITE_DEV_OP | OVS_READ_DEV_OP,
      .validateDpIndex  = FALSE
    }
};

NETLINK_FAMILY nlCtFamilyOps = {
    .name     = OVS_CT_FAMILY, /* Keep this for consistency*/
    .id       = OVS_WIN_NL_CT_FAMILY_ID, /* Keep this for consistency*/
    .version  = OVS_CT_VERSION, /* Keep this for consistency*/
    .maxAttr  = OVS_NL_CT_ATTR_MAX,
    .cmds     = nlCtFamilyCmdOps,
    .opsCount = ARRAY_SIZE(nlCtFamilyCmdOps)
};


The problem with tying up OVS_WIN_NL_CT_FAMILY_ID to these commands meant
additional changes in userspace. Userspace currently uses the
NETLINK_NETFILTER for ct messages and follows a different pattern from
NETLINK_GENERIC (which is what we currently use). Eg - the
ovsMsg->nlMsg.nlmsgType is actually split into 2 - Subsystem + CMD
(NFNL_SUBSYS_CTNETLINK << 8 | IPCTNL_MSG_CT_GET). Since there was already
support for this format in userspace, I didn¹t want to add too many #ifdef
statement around the message creation.

My original thought was to add OVS_WIN_NL_CT_FAMILY_ID in userspace and
add support for it in do_lookup_genl_family() in netlink-socket.c. That
would mean modifying dpif-netlink.c along with netlink-conntrack.c. This
approach left me with a code that had too many #ifdef in userspace. The
current approach limited the userspace changes while passing along the
necessary info. If there is a simpler alternative, am open to implementing
that.

Thanks,
Sairam

On 6/23/16, 12:50 PM, "Paul Boca" <pboca@cloudbasesolutions.com> wrote:

>Hi Sai!
>
>
>
>Please see my comments inline.
>
>Besides that it looks good to me.
>
>
>
>Thanks,
>
>Paul
>
>
>
>> -----Original Message-----
>
>> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Sairam
>
>> Venugopal
>
>> Sent: Tuesday, June 21, 2016 4:23 AM
>
>> To: dev@openvswitch.org
>
>> Subject: [ovs-dev] [PATCH 9/9] datapath-windows: Add support for
>
>> Conntrack IPCTNL_MSG_CT_GET cmd in Datapath.c
>
>> 
>
>> This will be used by userspace for dumping conntrack entries -
>>"ovs-dpctl
>
>> dump-conntrack".
>
>> 
>
>> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
>
>> ---
>
>>  datapath-windows/ovsext/Datapath.c | 12 ++++++++++--
>
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>
>> 
>
>> diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-
>
>> windows/ovsext/Datapath.c
>
>> index 7cc8390..5cc0614 100644
>
>> --- a/datapath-windows/ovsext/Datapath.c
>
>> +++ b/datapath-windows/ovsext/Datapath.c
>
>> @@ -104,7 +104,8 @@ NetlinkCmdHandler        OvsGetNetdevCmdHandler,
>
>>                           OvsPendPacketCmdHandler,
>
>>                           OvsSubscribePacketCmdHandler,
>
>>                           OvsReadPacketCmdHandler,
>
>> -                         OvsCtDeleteCmdHandler;
>
>> +                         OvsCtDeleteCmdHandler,
>
>> +                         OvsCtDumpCmdHandler;
>
>> 
>
>>  static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT
>
>> usrParamsCtx,
>
>>                                         UINT32 *replyLen);
>
>> @@ -288,7 +289,13 @@ NETLINK_CMD nlCtFamilyCmdOps[] = {
>
>>      { .cmd              = IPCTNL_MSG_CT_DELETE,
>
>>        .handler          = OvsCtDeleteCmdHandler,
>
>>        .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
>
>> -      .validateDpIndex  = TRUE
>
>> +      .validateDpIndex  = FALSE
>
>> +    },
>
>> +    { .cmd              = IPCTNL_MSG_CT_GET,
>
>> +      .handler          = OvsCtDumpCmdHandler,
>
>> +      .supportedDevOp   = OVS_TRANSACTION_DEV_OP |
>
>> +                          OVS_WRITE_DEV_OP | OVS_READ_DEV_OP,
>
>> +      .validateDpIndex  = FALSE
>
>>      }
>
>>  };
>
>[Paul Boca] Now you have 2 NETLINK_CMDs, maybe it would be nice to declare
>
>a NETLINK_FAMILY which points to nlCtFamilyCmdOps. It is functional but it
>
>doesn't mentain the style used here.
>
>
>
>> 
>
>> @@ -904,6 +911,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
>
>> 
>
>>      ASSERT(ovsMsg);
>
>>      switch (ovsMsg->nlMsg.nlmsgType) {
>
>> +    case NFNL_TYPE_CT_GET:
>
>[Paul Boca] If you declare a new NETLINK_FAMILY then you will have here
>only 
>
>one case with OVS_WIN_..._FAMILY_ID
>
>
>
>>      case NFNL_TYPE_CT_DEL:
>
>>          nlFamilyOps = &nlCtFamilyOps;
>
>>          break;
>
>> --
>
>> 2.5.0.windows.1
>
>> 
>
>> _______________________________________________
>
>> dev mailing list
>
>> dev@openvswitch.org
>
>> 
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=
>>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=4kLod0hlZhjnb5LrbpgjM18Ex12
>>ofd9jGmS1WVJDMps&s=67iuH7TGw4dl4FD4m2PJ8Xl3v3JCDqCt13f30XQtRKs&e=
>
diff mbox

Patch

diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 7cc8390..5cc0614 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -104,7 +104,8 @@  NetlinkCmdHandler        OvsGetNetdevCmdHandler,
                          OvsPendPacketCmdHandler,
                          OvsSubscribePacketCmdHandler,
                          OvsReadPacketCmdHandler,
-                         OvsCtDeleteCmdHandler;
+                         OvsCtDeleteCmdHandler,
+                         OvsCtDumpCmdHandler;
 
 static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                                        UINT32 *replyLen);
@@ -288,7 +289,13 @@  NETLINK_CMD nlCtFamilyCmdOps[] = {
     { .cmd              = IPCTNL_MSG_CT_DELETE,
       .handler          = OvsCtDeleteCmdHandler,
       .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
-      .validateDpIndex  = TRUE
+      .validateDpIndex  = FALSE
+    },
+    { .cmd              = IPCTNL_MSG_CT_GET,
+      .handler          = OvsCtDumpCmdHandler,
+      .supportedDevOp   = OVS_TRANSACTION_DEV_OP |
+                          OVS_WRITE_DEV_OP | OVS_READ_DEV_OP,
+      .validateDpIndex  = FALSE
     }
 };
 
@@ -904,6 +911,7 @@  OvsDeviceControl(PDEVICE_OBJECT deviceObject,
 
     ASSERT(ovsMsg);
     switch (ovsMsg->nlMsg.nlmsgType) {
+    case NFNL_TYPE_CT_GET:
     case NFNL_TYPE_CT_DEL:
         nlFamilyOps = &nlCtFamilyOps;
         break;