hi Ankur,
Thanks for working on this. Looks good overall, but for the minor comments I
had.
On Aug 20, 2014, at 9:46 AM, Ankur Sharma <ankursha...@vmware.com> wrote:
> In this patch we incorporate the usage of netlink message
> and the parsing APIs that were added in previous commit.
>
> Change-Id: If627ba5a10d78b93668afd32da77807394cd06be
Like in the other commit, this needs to go.
> ---
> datapath-windows/include/OvsPub.h | 13 +++-----
> datapath-windows/ovsext/Datapath.c | 8 ++---
> datapath-windows/ovsext/Datapath.h | 4 +--
> datapath-windows/ovsext/OvsActions.c | 57 ++++++++++++++++++-----------------
> datapath-windows/ovsext/OvsDebug.h | 1 +
> datapath-windows/ovsext/OvsFlow.h | 2 +-
> datapath-windows/ovsext/OvsPacketIO.h | 2 +-
> datapath-windows/ovsext/OvsUser.c | 4 +--
> 8 files changed, 44 insertions(+), 47 deletions(-)
>
> diff --git a/datapath-windows/include/OvsPub.h
> b/datapath-windows/include/OvsPub.h
> index 1282996..0446309 100644
> --- a/datapath-windows/include/OvsPub.h
> +++ b/datapath-windows/include/OvsPub.h
> @@ -17,12 +17,7 @@
> #ifndef __OVS_PUB_H_
> #define __OVS_PUB_H_ 1
>
> `-/* Needed by netlink-protocol.h */
> -#define BUILD_ASSERT(EXPR) \
> - typedef char AssertOnCompileFailed[(EXPR) ? 1: -1]
> -#define BUILD_ASSERT_DECL(EXPR) BUILD_ASSERT(EXPR)
> -
> -#include "OvsNetlink.h"
> +#include "../ovsext/Netlink.h"
> #define OVS_DRIVER_MAJOR_VER 1
> #define OVS_DRIVER_MINOR_VER 0
> @@ -369,7 +364,7 @@ typedef struct OvsFlowInfo {
> OvsFlowKey key;
> struct OvsFlowStats stats;
> uint32_t actionsLen;
> - struct nlattr actions[0];
> + NL_ATTR actions[0];
> } OvsFlowInfo;
>
> enum GetFlags {
> @@ -425,7 +420,7 @@ typedef struct OvsFlowPut {
> uint32_t actionsLen;
> OvsFlowKey key;
> uint32_t flags;
> - struct nlattr actions[0]; /* Variable length indicated by actionsLen.
> */
> + NL_ATTR actions[0]; /* Variable length indicated by actionsLen. */
> } OvsFlowPut;
>
> #define OVS_MIN_PACKET_SIZE 60
> @@ -452,7 +447,7 @@ typedef struct OvsPacketExecute {
> /* Variable size blob with packet data first, followed by action
> * attrs. */
> char packetBuf[0];
> - struct nlattr actions[0];
> + NL_ATTR actions[0];
> };
> } OvsPacketExecute;
LG
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index 3bb2a2a..40654f5 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -529,7 +529,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> PIO_STACK_LOCATION irpSp;
> NTSTATUS status = STATUS_SUCCESS;
> PFILE_OBJECT fileObject;
> - PVOID inputBuffer;
> + PVOID inputBuffer = NULL;
> PVOID outputBuffer = NULL;
> UINT32 inputBufferLen, outputBufferLen;
> UINT32 code, replyLen = 0;
> @@ -645,7 +645,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> }
>
> ASSERT(ovsMsg);
> - switch (ovsMsg->nlMsg.nlmsg_type) {
> + switch (ovsMsg->nlMsg.nlmsgType) {
> case OVS_WIN_NL_CTRL_FAMILY_ID:
> nlFamilyOps = &nlControlFamilyOps;
> break;
> @@ -801,8 +801,8 @@ OvsGetPidCmdHandler(PIRP irp,
> POVS_OPEN_INSTANCE instance =
> (POVS_OPEN_INSTANCE)fileObject->FsContext;
>
> RtlZeroMemory(msgOut, sizeof *msgOut);
> - msgOut->nlMsg.nlmsg_seq = msgIn->nlMsg.nlmsg_seq;
> - msgOut->nlMsg.nlmsg_pid = instance->pid;
> + msgOut->nlMsg.nlmsgSeq = msgIn->nlMsg.nlmsgSeq;
> + msgOut->nlMsg.nlmsgPid = instance->pid;
> *replyLen = sizeof *msgOut;
> /* XXX: We might need to return the DP index as well. */
> } else {
LG
> diff --git a/datapath-windows/ovsext/Datapath.h
> b/datapath-windows/ovsext/Datapath.h
> index 2bea0fd..6d8a6db 100644
> --- a/datapath-windows/ovsext/Datapath.h
> +++ b/datapath-windows/ovsext/Datapath.h
> @@ -71,8 +71,8 @@ NTSTATUS OvsCompleteIrpRequest(PIRP irp, ULONG_PTR infoPtr,
> NTSTATUS status);
> * Structure of any message passed between userspace and kernel.
> */
> typedef struct _OVS_MESSAGE {
> - struct nlmsghdr nlMsg;
> - struct genlmsghdr genlMsg;
> + NL_MSG_HDR nlMsg;
> + GENL_MSG_HDR genlMsg;
> struct ovs_header ovsHdr;
> /* Variable length nl_attrs follow. */
> } OVS_MESSAGE, *POVS_MESSAGE;
LG
> diff --git a/datapath-windows/ovsext/OvsActions.c
> b/datapath-windows/ovsext/OvsActions.c
> index d62f2a7..7d08d7e 100644
> --- a/datapath-windows/ovsext/OvsActions.c
> +++ b/datapath-windows/ovsext/OvsActions.c
> @@ -25,7 +25,7 @@
> #include "OvsVxlan.h"
> #include "OvsChecksum.h"
> #include "OvsPacketIO.h"
> -
> +#include "Netlink.h"
We include precomp.h as the first file which already includes Netlink.h. So, we
don't need the explicit include.
> #ifdef OVS_DBG_MOD
> #undef OVS_DBG_MOD
> @@ -1030,35 +1030,35 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
> * --------------------------------------------------------------------------
> */
> static __inline NDIS_STATUS
> -OvsTunnelAttrToIPv4TunnelKey(struct nlattr *attr,
> +OvsTunnelAttrToIPv4TunnelKey(PNL_ATTR attr,
> OvsIPv4TunnelKey *tunKey)
> {
> - struct nlattr *a;
> + PNL_ATTR a;
> INT rem;
>
> tunKey->attr[0] = 0;
> tunKey->attr[1] = 0;
> tunKey->attr[2] = 0;
> - ASSERT(nl_attr_type(attr) == OVS_KEY_ATTR_TUNNEL);
> + ASSERT(NlAttrType(attr) == OVS_KEY_ATTR_TUNNEL);
>
> - NL_ATTR_FOR_EACH_UNSAFE (a, rem, nl_attr_data(attr),
> - nl_attr_get_size(attr)) {
> - switch (nl_attr_type(a)) {
> + NL_ATTR_FOR_EACH_UNSAFE (a, rem, NlAttrData(attr),
> + NlAttrGetSize(attr)) {
> + switch (NlAttrType(a)) {
> case OVS_TUNNEL_KEY_ATTR_ID:
> - tunKey->tunnelId = nl_attr_get_be64(a);
> + tunKey->tunnelId = NlAttrGetBe64(a);
> tunKey->flags |= OVS_TNL_F_KEY;
> break;
> case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
> - tunKey->src = nl_attr_get_be32(a);
> + tunKey->src = NlAttrGetBe32(a);
> break;
> case OVS_TUNNEL_KEY_ATTR_IPV4_DST:
> - tunKey->dst = nl_attr_get_be32(a);
> + tunKey->dst = NlAttrGetBe32(a);
> break;
> case OVS_TUNNEL_KEY_ATTR_TOS:
> - tunKey->tos = nl_attr_get_u8(a);
> + tunKey->tos = NlAttrGetU8(a);
> break;
> case OVS_TUNNEL_KEY_ATTR_TTL:
> - tunKey->ttl = nl_attr_get_u8(a);
> + tunKey->ttl = NlAttrGetU8(a);
> break;
> case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT:
> tunKey->flags |= OVS_TNL_F_DONT_FRAGMENT;
LG
> @@ -1283,27 +1283,27 @@ static __inline NDIS_STATUS
> OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,
> OvsFlowKey *key,
> UINT64 *hash,
> - const struct nlattr *a)
> + const PNL_ATTR a)
> {
> - enum ovs_key_attr type = nl_attr_type(a);
> + enum ovs_key_attr type = NlAttrType(a);
> NDIS_STATUS status = NDIS_STATUS_SUCCESS;
>
> switch (type) {
> case OVS_KEY_ATTR_ETHERNET:
> status = OvsUpdateEthHeader(ovsFwdCtx,
> - nl_attr_get_unspec(a, sizeof(struct ovs_key_ethernet)));
> + NlAttrGetUnspec(a, sizeof(struct ovs_key_ethernet)));
> break;
>
> case OVS_KEY_ATTR_IPV4:
> status = OvsUpdateIPv4Header(ovsFwdCtx,
> - nl_attr_get_unspec(a, sizeof(struct ovs_key_ipv4)));
> + NlAttrGetUnspec(a, sizeof(struct ovs_key_ipv4)));
> break;
>
> case OVS_KEY_ATTR_TUNNEL:
> {
> OvsIPv4TunnelKey tunKey;
>
> - status = OvsTunnelAttrToIPv4TunnelKey((struct nlattr *)a,
> &tunKey);
> + status = OvsTunnelAttrToIPv4TunnelKey((PNL_ATTR)a, &tunKey);
> ASSERT(status == NDIS_STATUS_SUCCESS);
> tunKey.flow_hash = (uint16)(hash ? *hash : OvsHashFlow(key));
> RtlCopyMemory(&ovsFwdCtx->tunKey, &tunKey, sizeof ovsFwdCtx->tunKey);
LG
> @@ -1357,10 +1357,10 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
> OvsFlowKey *key,
> UINT64 *hash,
> OVS_PACKET_HDR_INFO *layers,
> - const struct nlattr *actions,
> + const PNL_ATTR actions,
> INT actionsLen)
> {
> - const struct nlattr *a;
> + PNL_ATTR a;
> INT rem;
> UINT32 dstPortID;
> OvsForwardingContext ovsFwdCtx;
> @@ -1385,9 +1385,9 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
> }
>
> NL_ATTR_FOR_EACH_UNSAFE (a, rem, actions, actionsLen) {
> - switch(nl_attr_type(a)) {
> + switch(NlAttrType(a)) {
> case OVS_ACTION_ATTR_OUTPUT:
> - dstPortID = nl_attr_get_u32(a);
> + dstPortID = NlAttrGetU32(a);
> status = OvsAddPorts(&ovsFwdCtx, key, dstPortID,
> TRUE, TRUE);
> if (status != NDIS_STATUS_SUCCESS) {
> @@ -1424,7 +1424,7 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
> } else {
> vlanTagValue = 0;
> vlanTag = (PNDIS_NET_BUFFER_LIST_8021Q_INFO)(PVOID
> *)&vlanTagValue;
> - vlan = (struct ovs_action_push_vlan *)nl_attr_get(a);
> + vlan = (struct ovs_action_push_vlan *)NlAttrGet((const
> PNL_ATTR)a);
> vlanTag->TagHeader.VlanId = ntohs(vlan->vlan_tci) & 0xfff;
> vlanTag->TagHeader.UserPriority = ntohs(vlan->vlan_tci) >> 13;
>
> @@ -1465,19 +1465,19 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
>
> case OVS_ACTION_ATTR_USERSPACE:
> {
> - const struct nlattr *userdata_attr;
> - const struct nlattr *queue_attr;
> + PNL_ATTR userdata_attr;
> + PNL_ATTR queue_attr;
userdata_attr => userdataAttr
queue_attr => queueAttr
(this is not introduced by your change)
> POVS_PACKET_QUEUE_ELEM elem;
> UINT32 queueId = OVS_DEFAULT_PACKET_QUEUE;
> //XXX confusing that portNo is actually portId for external port.
> BOOLEAN isRecv = (portNo == switchContext->externalPortId)
> || OvsIsTunnelVportNo(portNo);
>
> - queue_attr = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_PID);
> - userdata_attr = nl_attr_find_nested(a,
> OVS_USERSPACE_ATTR_USERDATA);
> + queue_attr = NlAttrFindNested(a, OVS_USERSPACE_ATTR_PID);
> + userdata_attr = NlAttrFindNested(a, OVS_USERSPACE_ATTR_USERDATA);
>
> elem = OvsCreateQueuePacket(queueId, (PVOID)userdata_attr,
> - userdata_attr->nla_len,
> + userdata_attr->nlaLen,
> OVS_PACKET_CMD_ACTION,
> portNo, (OvsIPv4TunnelKey
> *)&key->tunKey,
> ovsFwdCtx.curNbl,
> @@ -1510,7 +1510,8 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
> }
>
> status = OvsExecuteSetAction(&ovsFwdCtx, key, hash,
> - nl_attr_get(a));
> + (const PNL_ATTR)NlAttrGet
> + ((const PNL_ATTR)a));
In terms of indentation, best to shift this line to the left. 4 spaces to the
right of OvsExecuteSetAction.
> if (status != NDIS_STATUS_SUCCESS) {
> dropReason = L"OVS-set action failed";
> goto dropit;
LG
> diff --git a/datapath-windows/ovsext/OvsDebug.h
> b/datapath-windows/ovsext/OvsDebug.h
> index 6941f13..a57e73e 100644
> --- a/datapath-windows/ovsext/OvsDebug.h
> +++ b/datapath-windows/ovsext/OvsDebug.h
> @@ -38,6 +38,7 @@
> #define OVS_DBG_IPHELPER BIT32(18)
> #define OVS_DBG_BUFMGMT BIT32(19)
> #define OVS_DBG_OTHERS BIT32(21)
> +#define OVS_DBG_NETLINK BIT32(22)
>
> #define OVS_DBG_RESERVED BIT32(31)
> //Please add above OVS_DBG_RESERVED.
LG.
> diff --git a/datapath-windows/ovsext/OvsFlow.h
> b/datapath-windows/ovsext/OvsFlow.h
> index 93368b3..fa29c68 100644
> --- a/datapath-windows/ovsext/OvsFlow.h
> +++ b/datapath-windows/ovsext/OvsFlow.h
> @@ -33,7 +33,7 @@ typedef struct _OvsFlow {
> UINT64 byteCount;
> UINT32 userActionsLen; // used for flow query
> UINT32 actionBufferLen; // used for flow reuse
> - struct nlattr actions[1];
> + NL_ATTR actions[1];
> } OvsFlow;
>
LG
> diff --git a/datapath-windows/ovsext/OvsPacketIO.h
> b/datapath-windows/ovsext/OvsPacketIO.h
> index 322a8aa..11709dc 100644
> --- a/datapath-windows/ovsext/OvsPacketIO.h
> +++ b/datapath-windows/ovsext/OvsPacketIO.h
> @@ -53,7 +53,7 @@ NDIS_STATUS OvsActionsExecute(POVS_SWITCH_CONTEXT
> switchContext,
> PNET_BUFFER_LIST curNbl, UINT32 srcVportNo,
> ULONG sendFlags, OvsFlowKey *key, UINT64 *hash,
> OVS_PACKET_HDR_INFO *layers,
> - const struct nlattr *actions, int actionsLen);
> + const PNL_ATTR actions, int actionsLen);
>
> VOID OvsLookupFlowOutput(POVS_SWITCH_CONTEXT switchContext,
> VOID *compList, PNET_BUFFER_LIST curNbl);
> diff --git a/datapath-windows/ovsext/OvsUser.c
> b/datapath-windows/ovsext/OvsUser.c
> index 9fafb16..aac45ae 100644
> --- a/datapath-windows/ovsext/OvsUser.c
> +++ b/datapath-windows/ovsext/OvsUser.c
> @@ -310,7 +310,7 @@ OvsExecuteDpIoctl(PVOID inputBuffer,
> OvsPacketExecute *execute;
> LOCK_STATE_EX lockState;
> PNET_BUFFER_LIST pNbl;
> - struct nlattr *actions;
> + PNL_ATTR actions;
> PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO fwdDetail;
> OvsFlowKey key;
> OVS_PACKET_HDR_INFO layers;
> @@ -338,7 +338,7 @@ OvsExecuteDpIoctl(PVOID inputBuffer,
> status = STATUS_INFO_LENGTH_MISMATCH;
> goto unlock;
> }
> - actions = (struct nlattr *)((PCHAR)&execute->actions +
> execute->packetLen);
> + actions = (PNL_ATTR)((PCHAR)&execute->actions + execute->packetLen);
>
> /*
> * Allocate the NBL, copy the data from the userspace buffer. Allocate
LG
thanks,
Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
