Thanks for the review. I will send out a V2 addressing the changes.
On 7/8/16, 6:06 PM, "Nithin Raju" <nit...@vmware.com> wrote:
>Thanks for doing this. Looks good but for a few cosmetic comments.
>
>Also, remember to flip the protocol for nf sockets in userspace to
>NETLINK_NETFILTER. IIRC, we use NETLINK_GENERIC.
>
>Acked-by: Nithin Raju <nit...@vmware.com>
>
>-----Original Message-----
>From: dev <dev-boun...@openvswitch.org> on behalf of Sairam Venugopal
><vsai...@vmware.com>
>Date: Thursday, July 7, 2016 at 6:14 PM
>To: "dev@openvswitch.org" <dev@openvswitch.org>
>Subject: [ovs-dev] [PATCH] Windows: Add support for handling
>protocol (netlink family)
>
>>Windows datapath currently has no notion of netlink family.
>>It assumes all netlink messages to belong to NETLINK_GENERIC family.
>>This patch adds support for handling other protocols if the userspace
>>sends it down to kernel.
>>
>>This patch introduces a new NETLINK_CMD - OVS_CTRL_CMD_SOCK_PROP to
>>manage
>>all properties associated with a socket. The properties are passed down
>>as
>>netlink message attributes. This makes it easier to introduce other
>>properties in the future.
>>
>>Signed-off-by: Sairam Venugopal <vsai...@vmware.com>
>>---
>> datapath-windows/include/OvsDpInterfaceExt.h | 15 +++-
>> datapath-windows/ovsext/Datapath.c | 102
>>++++++++++++++++++++++++++-
>> datapath-windows/ovsext/Datapath.h | 1 +
>> lib/netlink-socket.c | 75 ++++++++++++++++++++
>> 4 files changed, 191 insertions(+), 2 deletions(-)
>>
>>diff --git a/datapath-windows/include/OvsDpInterfaceExt.h
>>b/datapath-windows/include/OvsDpInterfaceExt.h
>>index 8850535..e75ce28 100644
>>--- a/datapath-windows/include/OvsDpInterfaceExt.h
>>+++ b/datapath-windows/include/OvsDpInterfaceExt.h
>>@@ -94,7 +94,10 @@ enum ovs_win_control_cmd {
>>
>> /* This command is logically belong to the Vport family */
>
>minor: ³is logically belong² => ³logically belongs²
>(not your code)
>
>> OVS_CTRL_CMD_EVENT_NOTIFY,
>>- OVS_CTRL_CMD_READ_NOTIFY
>>+ OVS_CTRL_CMD_READ_NOTIFY,
>>+
>>+ /* Used for Socket property */
>>+ OVS_CTRL_CMD_SOCK_PROP
>> };
>>
>> /* NL Attributes for joining/unjoining an MC group */
>>@@ -163,4 +166,14 @@ enum ovs_win_netdev_attr {
>> typedef struct ovs_dp_stats OVS_DP_STATS;
>> typedef enum ovs_vport_type OVS_VPORT_TYPE;
>>
>>+/* NL Attributes for setting socket attributes */
>>+enum ovs_nl_sock_attr {
>>+ /* (UINT32) Netlink Protocol set in Userspace and read in Kernel */
>>+ OVS_NL_ATTR_SOCK_PROTO,
>>+ /* (UINT32) Instance PID set in Kernel and read in Userspace */
>>+ OVS_NL_ATTR_SOCK_PID,
>>+ __OVS_NL_ATTR_SOCK_MAX
>>+};
>>+#define OVS_WIN_SOCK_ATTR_MAX (__OVS_NL_ATTR_SOCK_MAX - 1)
>>+
>> #endif /* __OVS_DP_INTERFACE_EXT_H_ */
>>diff --git a/datapath-windows/ovsext/Datapath.c
>>b/datapath-windows/ovsext/Datapath.c
>>index 16d58ef..210184d 100644
>>--- a/datapath-windows/ovsext/Datapath.c
>>+++ b/datapath-windows/ovsext/Datapath.c
>>@@ -87,7 +87,8 @@ static NetlinkCmdHandler OvsPendEventCmdHandler,
>> OvsReadEventCmdHandler,
>> OvsNewDpCmdHandler,
>> OvsGetDpCmdHandler,
>>- OvsSetDpCmdHandler;
>>+ OvsSetDpCmdHandler,
>>+ OvsManageSocketProperty;
>
>Minor: All NL handlers are called: *CmdHandler. So, maybe
>OvsSockPropCmdHandler().
>
>>
>> NetlinkCmdHandler OvsGetNetdevCmdHandler,
>> OvsGetVportCmdHandler,
>>@@ -147,6 +148,11 @@ NETLINK_CMD nlControlFamilyCmdOps[] = {
>> .handler = OvsReadPacketCmdHandler,
>> .supportedDevOp = OVS_READ_DEV_OP,
>> .validateDpIndex = FALSE,
>>+ },
>>+ { .cmd = OVS_CTRL_CMD_SOCK_PROP,
>>+ .handler = OvsManageSocketProperty,
>>+ .supportedDevOp = OVS_TRANSACTION_DEV_OP,
>>+ .validateDpIndex = FALSE,
>> }
>> };
>>
>>@@ -1685,3 +1691,97 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT
>>usrParamsCtx,
>> cleanup:
>> return status;
>> }
>>+
>>+/*
>>+ *
>>-------------------------------------------------------------------------
>>-
>>+ * Command Handler for 'OVS_CTRL_CMD_SOCK_PROP'.
>>+ *
>>+ * Handler to set and verify socket properties between userspace and
>>kernel.
>>+ *
>>+ * Protocol is passed down by the userspace. It refers to the NETLINK
>>family
>>+ * and could be of different types (NETLINK_GENERIC/NETLINK_NETFILTER
>>etc.,)
>>+ * This function parses out the protocol and adds it to the open
>>instance.
>>+ *
>>+ * PID is generated by the kernel and is set in userspace after
>>querying the
>>+ * kernel for it. This function does not modify PID set in the kernel,
>>+ * instead it verifies if it was sent down correctly.
>>+ *
>>+ * XXX -This method can be modified to handle all Socket properties
>>thereby
>>+ * eliminating the use of OVS_IOCTL_GET_PID
>>+ *
>>+ *
>>-------------------------------------------------------------------------
>>-
>>+ */
>>+static NTSTATUS
>>+OvsManageSocketProperty(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>>+ UINT32 *replyLen)
>>+{
>>+ static const NL_POLICY ovsSocketPolicy[] = {
>>+ [OVS_NL_ATTR_SOCK_PROTO] = { .type = NL_A_U32, .optional = TRUE
>>},
>>+ [OVS_NL_ATTR_SOCK_PID] = { .type = NL_A_U32, .optional = TRUE }
>>+ };
>>+ PNL_ATTR attrs[ARRAY_SIZE(ovsSocketPolicy)];
>>+
>>+ if (usrParamsCtx->outputLength < sizeof(OVS_MESSAGE)) {
>>+ return STATUS_NDIS_INVALID_LENGTH;
>>+ }
>>+
>>+ NL_BUFFER nlBuf;
>>+ POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
>>+ POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
>>+ POVS_OPEN_INSTANCE instance =
>>(POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;
>>+ UINT32 protocol;
>>+ PNL_MSG_HDR nlMsg;
>>+ UINT32 pid;
>>+
>>+ /* Parse the input */
>>+ if (!NlAttrParse((PNL_MSG_HDR)msgIn,
>>+ NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN,
>>+ NlMsgAttrsLen((PNL_MSG_HDR)msgIn),
>>+ ovsSocketPolicy, ARRAY_SIZE(ovsSocketPolicy),
>>+ attrs, ARRAY_SIZE(attrs))) {
>>+ return STATUS_INVALID_PARAMETER;
>>+ }
>
>minor: align arg #2 onwards either 4 spaces to the right or along the same
>level as arg #1.
>
>>+
>>+ /* Set the Protocol if it was passed down */
>>+ if (attrs[OVS_NL_ATTR_SOCK_PROTO]) {
>>+ protocol = NlAttrGetU32(attrs[OVS_NL_ATTR_SOCK_PROTO]);
>>+ if (protocol) {
>>+ instance->protocol = protocol;
>>+ }
>>+ }
>>+
>>+ /* Verify if the PID sent down matches the kernel */
>>+ if (attrs[OVS_NL_ATTR_SOCK_PID]) {
>>+ pid = NlAttrGetU32(attrs[OVS_NL_ATTR_SOCK_PID]);
>>+ if (pid != instance->pid) {
>>+ OVS_LOG_ERROR("Received invalid pid:%d expected:%d",
>>+ pid, instance->pid);
>>+ return STATUS_INVALID_PARAMETER;
>>+ }
>>+ }
>>+
>>+ /* Prepare the output */
>>+ NlBufInit(&nlBuf, usrParamsCtx->outputBuffer,
>>usrParamsCtx->outputLength);
>>+ if(!NlFillOvsMsg(&nlBuf, msgIn->nlMsg.nlmsgType, NLM_F_MULTI,
>>+ msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid,
>>+ msgIn->genlMsg.cmd, msgIn->genlMsg.version,
>>+ gOvsSwitchContext->dpNo)){
>>+ return STATUS_INVALID_BUFFER_SIZE;
>>+ }
>>+
>>+ if (!NlMsgPutTailU32(&nlBuf, OVS_NL_ATTR_SOCK_PID,
>>+ instance->pid)) {
>>+ return STATUS_INVALID_BUFFER_SIZE;
>>+ }
>>+
>>+ if (!NlMsgPutTailU32(&nlBuf, OVS_NL_ATTR_SOCK_PROTO,
>>+ instance->protocol)) {
>>+ return STATUS_INVALID_BUFFER_SIZE;
>>+ }
>>+
>>+ nlMsg = (PNL_MSG_HDR)NlBufAt(&nlBuf, 0, 0);
>>+ nlMsg->nlmsgLen = NlBufSize(&nlBuf);
>>+ *replyLen = msgOut->nlMsg.nlmsgLen;
>>+
>>+ return STATUS_SUCCESS;
>>+}
>>diff --git a/datapath-windows/ovsext/Datapath.h
>>b/datapath-windows/ovsext/Datapath.h
>>index 09e233f..2b41d82 100644
>>--- a/datapath-windows/ovsext/Datapath.h
>>+++ b/datapath-windows/ovsext/Datapath.h
>>@@ -51,6 +51,7 @@ typedef struct _OVS_OPEN_INSTANCE {
>> PVOID eventQueue;
>> POVS_USER_PACKET_QUEUE packetQueue;
>> UINT32 pid;
>>+ UINT32 protocol; /* Refers to NETLINK Family (eg. NETLINK_GENERIC)*/
>>
>> struct {
>> POVS_MESSAGE ovsMsg; /* OVS message passed during dump start.
>>*/
>>diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
>>index 32b0cc3..51375d4 100644
>>--- a/lib/netlink-socket.c
>>+++ b/lib/netlink-socket.c
>>@@ -59,6 +59,7 @@ static void log_nlmsg(const char *function, int error,
>> const void *message, size_t size, int protocol);
>> #ifdef _WIN32
>> static int get_sock_pid_from_kernel(struct nl_sock *sock);
>>+static int set_sock_property(struct nl_sock *sock);
>> #endif
>> ?
>> /* Netlink sockets. */
>>@@ -165,6 +166,10 @@ nl_sock_create(int protocol, struct nl_sock **sockp)
>> if (retval != 0) {
>> goto error;
>> }
>>+ retval = set_sock_property(sock);
>>+ if (retval != 0) {
>>+ goto error;
>>+ }
>> #else
>> if (setsockopt(sock->fd, SOL_SOCKET, SO_RCVBUFFORCE,
>> &rcvbuf, sizeof rcvbuf)) {
>>@@ -284,6 +289,76 @@ get_sock_pid_from_kernel(struct nl_sock *sock)
>>
>> return retval;
>> }
>>+
>>+/* Used for setting and managing socket properties in userspace and
>>kernel.
>>+ * Currently two attributes are tracked - pid and protocol
>>+ * protocol - supplied by userspace based on the netlink family. Windows
>>uses
>>+ * this property to set the value in kernel datapath.
>>+ * eg: (NETLINK_GENERIC/ NETLINK_NETFILTER)
>>+ * pid - generated by windows kernel and set in userspace. The
>>property
>>+ * is not modified.
>>+ * Also verify if Protocol and PID in Kernel reflects the values in
>>userspace
>>+ * */
>>+static int
>>+set_sock_property(struct nl_sock *sock)
>>+{
>>+ static const struct nl_policy ovs_socket_policy[] = {
>>+ [OVS_NL_ATTR_SOCK_PROTO] = { .type = NL_A_BE32, .optional = true
>>},
>>+ [OVS_NL_ATTR_SOCK_PID] = { .type = NL_A_BE32, .optional = true }
>>+ };
>>+
>>+ struct ofpbuf request, *reply;
>>+ struct ovs_header *ovs_header;
>>+ struct nlattr *attrs[ARRAY_SIZE(ovs_socket_policy)];
>>+ int retval = 0;
>>+ int error;
>>+
>>+ ofpbuf_init(&request, 0);
>>+ nl_msg_put_genlmsghdr(&request, 0, OVS_WIN_NL_CTRL_FAMILY_ID, 0,
>>+ OVS_CTRL_CMD_SOCK_PROP,
>>OVS_WIN_CONTROL_VERSION);
>>+ ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header);
>>+ ovs_header->dp_ifindex = 0;
>>+
>>+ nl_msg_put_be32(&request, OVS_NL_ATTR_SOCK_PROTO, sock->protocol);
>>+ /* pid is already set as part of get_sock_pid_from_kernel()
>>+ * This is added to maintain consistency
>>+ */
>>+ nl_msg_put_be32(&request, OVS_NL_ATTR_SOCK_PID, sock->pid);
>>+
>>+ error = nl_sock_transact(sock, &request, &reply);
>>+ ofpbuf_uninit(&request);
>>+ if (error) {
>>+ retval = EINVAL;
>>+ }
>>+
>>+ if (!nl_policy_parse(reply,
>>+ NLMSG_HDRLEN + GENL_HDRLEN + sizeof
>>*ovs_header,
>>+ ovs_socket_policy, attrs,
>>+ ARRAY_SIZE(ovs_socket_policy))) {
>>+ ofpbuf_delete(reply);
>>+ retval = EINVAL;
>>+ }
>>+ /* Verify if the properties are setup properly */
>>+ if(attrs[OVS_NL_ATTR_SOCK_PROTO]) {
>
>minor: whitespace needed after Œif'
>
>>+ int protocol = nl_attr_get_be32(attrs[OVS_NL_ATTR_SOCK_PROTO]);
>>+ if(protocol != sock->protocol) {
>
>minor: whitespace needed after Œif'
>
>
>>+ VLOG_ERR("Invalid protocol returned:%d expected:%d",
>>+ protocol, sock->protocol);
>>+ retval = EINVAL;
>>+ }
>>+ }
>>+
>>+ if(attrs[OVS_NL_ATTR_SOCK_PID]) {
>
>minor: whitespace needed after Œif'
>
>
>>+ int pid = nl_attr_get_be32(attrs[OVS_NL_ATTR_SOCK_PID]);
>>+ if(pid != sock->pid) {
>
>minor: whitespace needed after Œif'
>
>
>>+ VLOG_ERR("Invalid pid returned:%d expected:%d",
>>+ pid, sock->pid);
>>+ retval = EINVAL;
>>+ }
>>+ }
>>+
>>+ return retval;
>>+}
>> #endif /* _WIN32 */
>>
>> #ifdef _WIN32
>>--
>>2.9.0.windows.1
>>
>>_______________________________________________
>>dev mailing list
>>dev@openvswitch.org
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>a
>>n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=p
>>N
>>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=5RKcCre7K3WdKv0m8O_a7zgr-B9pr
>>f
>>gWhRJcc2afLps&s=hSVROJkjb846dj_5qSkSX-nXy_aghVtbqHu1cF40KjM&e=
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
