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" <pb...@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-boun...@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 <vsai...@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=
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
