[ovs-dev] [PATCH 2/3] datapath-windows: Using windows kernel netlink parsing APIs.

Nithin Raju nithin at vmware.com
Thu Aug 21 16:28:04 UTC 2014


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 <ankursharma at 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


More information about the dev mailing list