[ovs-dev] [PATCH 11/14] datapath-windows: Add netlink command: vport new
Nithin Raju
nithin at vmware.com
Sat Oct 4 17:43:38 UTC 2014
hi Alin,
This is a huge change, and I think you and Samuel are on the right track. I have noted down a few comments. Pls. address them and also the comments from Eitan and Ankur, and we should be in a good shape.
On Sep 30, 2014, at 7:58 AM, Samuel Ghinet <sghinet at cloudbasesolutions.com> wrote:
> Does the following:
> a. before creating the vport, makes sure there is no existing vport
> with the same ovs (datapath) port name. If this is not so, it means
> that the specified port already exists: it returns NL_ERROR_EXIST.
> b. looks up the vport:
> o) if the vport type is "internal", then the internal vport of the
> hyper-v switch is yielded.
> o) if the vport type is "netdev" and the vport ovs (datapath) name
> is "external", then the external vport is yielded. The switch can
> have only one "external" vport. The method of looking up the
> "external" port can be changed later, if a better method is found.
> o) if the vport type is "netdev" but the name is not "external",
> then a VM VNic is assumed, so the vport is looked up by hyper-v
> switch port friendly name.
> o) if none of the above, a tunneling vport type is expected, which
> in our case, at the moment, can only be the one vxlan vport. Only
> one vxlan vport is allowed, and it's saved in
> switchContext->vxlanVport. The tunneling vport is the only kind
> which is created in the netlink command vport new, because it does
> not have a hyper-v switch port counterpart.
> c. if the vport could not be found (non-tunneling vports), then the
> NL_ERROR_INVAL is returned to the userspace.
> d. if the vport was found, but it has a valid ovs (datapath) port
> number, it means that this port was already created by a netlink
> command vport new. Therefore, NL_ERROR_EXIST is returned to the
> userspace.
> e. if the netlink command vport new specified an ovs (datapath) port
> number, then it means that the userspace is trying to re-create a
> vport: that specified port number will be used. Otherwise, a new
> ovs (datapath) port number is computed and assigned to the vport.
> f. the ovsName field of the vport is set to the name given by the
> OVS_VPORT_ATTR_NAME netlink attribute. The ovsNameLen is no longer
> stored in the OVS_VPORT_ENTRY struct, because ovsName is
> null-terminated.
> g. the "portOptions" are set to the vport, if the attribute
> OVS_VPORT_ATTR_OPTIONS was given. Otherwise, it is set to NULL.
> portOptions is a PNL_ATTR, which is yet to be implemented. The
> only option available for now would be vxlan udp destination port,
> but we have a constant value there, so this option is not yet needed.
> h. the upcall pid is set to the vport.
> i. if the vport type is vxlan, then the vport pointer is also saved
> to switchContext->vxlanVport.
> j. Now that the ovs (datapath) port number and the ovs name were set,
> the vport can be added to the hash array of vports, hashed on ovs name
> and to the hash array of vports hashed by ovs (datapath) port number.
> k. the reply is yielded to the userspace.
>
> Signed-off-by: Samuel Ghinet <sghinet at cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Datapath.c | 235 ++++++++++++++++++++++++++++++++++++-
> datapath-windows/ovsext/Vport.c | 68 ++++-------
> datapath-windows/ovsext/Vport.h | 19 ++-
> datapath-windows/ovsext/Vxlan.c | 48 +++++---
> datapath-windows/ovsext/Vxlan.h | 4 +-
> 5 files changed, 298 insertions(+), 76 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
> index eec65b3..dc03961 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -33,6 +33,7 @@
> #include "NetProto.h"
> #include "Flow.h"
> #include "User.h"
> +#include "Vxlan.h"
>
> #ifdef OVS_DBG_MOD
> #undef OVS_DBG_MOD
> @@ -102,7 +103,8 @@ static NetlinkCmdHandler OvsGetPidCmdHandler,
> OvsSubscribeEventCmdHandler,
> OvsSetDpCmdHandler,
> OvsReadEventCmdHandler,
> - OvsGetVportCmdHandler;
> + OvsGetVportCmdHandler,
> + OvsNewVportCmdHandler;
>
> static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> UINT32 *replyLen);
> @@ -194,6 +196,11 @@ NETLINK_CMD nlVportFamilyCmdOps[] = {
> .supportedDevOp = OVS_WRITE_DEV_OP | OVS_READ_DEV_OP |
> OVS_TRANSACTION_DEV_OP,
> .validateDpIndex = TRUE
> + },
> + { .cmd = OVS_VPORT_CMD_NEW,
> + .handler = OvsNewVportCmdHandler,
> + .supportedDevOp = OVS_TRANSACTION_DEV_OP,
> + .validateDpIndex = TRUE
> }
> };
>
> @@ -1491,6 +1498,9 @@ OvsGetVport(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> POVS_VPORT_ENTRY vport = NULL;
> NL_ERROR nlError = NL_ERROR_SUCCESS;
> + PCHAR portName = NULL;
> + UINT32 portNameLen = 0;
> + UINT32 portNumber = OVS_DPPORT_NUMBER_INVALID;
>
> static const NL_POLICY ovsVportPolicy[] = {
> [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE },
> @@ -1523,12 +1533,17 @@ OvsGetVport(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> OvsReleaseCtrlLock();
>
> if (vportAttrs[OVS_VPORT_ATTR_NAME] != NULL) {
> - vport = OvsFindVportByOvsName(gOvsSwitchContext,
> - NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]),
> - NlAttrGetSize(vportAttrs[OVS_VPORT_ATTR_NAME]) - 1);
> + portName = NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]);
> + portNameLen = NlAttrGetSize(vportAttrs[OVS_VPORT_ATTR_NAME]);
> +
> + /* the port name is expected to be null-terminated */
> + ASSERT(portName[portNameLen - 1] == '\0');
> +
> + vport = OvsFindVportByOvsName(gOvsSwitchContext, portName);
> } else if (vportAttrs[OVS_VPORT_ATTR_PORT_NO] != NULL) {
> - vport = OvsFindVportByPortNo(gOvsSwitchContext,
> - NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_PORT_NO]));
> + portNumber = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_PORT_NO]);
> +
> + vport = OvsFindVportByPortNo(gOvsSwitchContext, portNumber);
> } else {
> nlError = NL_ERROR_INVAL;
> goto Cleanup;
> @@ -1588,6 +1603,214 @@ OvsGetVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>
> }
>
> +static UINT32
> +OvsComputeVportNo(POVS_SWITCH_CONTEXT switchContext)
> +{
> + /* we are not allowed to create the port OVS_DPPORT_LOCAL */
> + for (ULONG i = OVS_DPPORT_LOCAL + 1; i < MAXUINT16; ++i) {
> + POVS_VPORT_ENTRY vport;
> +
> + vport = OvsFindVportByPortNo(switchContext, i);
> + if (!vport) {
> + return i;
> + }
> + }
> +
> + return OVS_DPPORT_NUMBER_INVALID;
> +}
> +
> +static NTSTATUS
> +OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> + UINT32 *replyLen)
> +{
> + NDIS_STATUS status = STATUS_SUCCESS;
> + LOCK_STATE_EX lockState;
> +
> + NL_ERROR nlError = NL_ERROR_SUCCESS;
> + POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
> + POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> + POVS_VPORT_ENTRY vport = NULL;
> + PCHAR portName;
> + ULONG portNameLen;
> + UINT32 portType;
> + UINT32 hash;
> +
> + static const NL_POLICY ovsVportPolicy[] = {
> + [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE },
> + [OVS_VPORT_ATTR_TYPE] = { .type = NL_A_U32, .optional = FALSE },
> + [OVS_VPORT_ATTR_NAME] = { .type = NL_A_STRING, .maxLen = IFNAMSIZ,
> + .optional = FALSE},
> + [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NL_A_UNSPEC,
> + .optional = FALSE },
> + [OVS_VPORT_ATTR_OPTIONS] = { .type = NL_A_NESTED, .optional = TRUE },
> + };
> +
> + PNL_ATTR vportAttrs[ARRAY_SIZE(ovsVportPolicy)];
> +
> + /* input buffer has been validated while validating write dev op. */
> + ASSERT(usrParamsCtx->inputBuffer != NULL);
> +
> + if (msgOut == NULL || usrParamsCtx->outputLength < sizeof *msgOut) {
> + return STATUS_INVALID_BUFFER_SIZE;
> + }
> +
> + if (!NlAttrParse((PNL_MSG_HDR)msgIn,
> + NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN,
You'll also have to pass the length of the attributes since there's a new argument to NlAttrParse().
> + ovsVportPolicy, vportAttrs, ARRAY_SIZE(vportAttrs))) {
> + return STATUS_INVALID_PARAMETER;
> + }
> +
> + OvsAcquireCtrlLock();
> + if (!gOvsSwitchContext) {
> + OvsReleaseCtrlLock();
> + return STATUS_INVALID_PARAMETER;
> + }
> + OvsReleaseCtrlLock();
> +
> + portName = NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]);
> + portNameLen = NlAttrGetSize(vportAttrs[OVS_VPORT_ATTR_NAME]);
> + portType = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_TYPE]);
> +
> + /* we are expecting null terminated strings to be passed */
> + ASSERT(portName[portNameLen - 1] == '\0');
> +
> + NdisAcquireRWLockWrite(gOvsSwitchContext->dispatchLock, &lockState, 0);
> +
> + vport = OvsFindVportByOvsName(gOvsSwitchContext, portName);
> + if (vport) {
> + nlError = NL_ERROR_EXIST;
> + goto Cleanup;
> + }
Also, you need to run a check using OvsFindVportByPortNo(), and if the names are same return NL_ERROR_EXIST, otherwise return 'invalid parameter'.
I don't see the usage of 'hvDeleted' field here. We should be checkin if 'hvDeleted' != FALSE before proceeding.
An alternative to using 'hvDeleted' flag is to look at 'vport->ovsState'. To start with, we should only consider ports that are in OVS_STATE_PORT_CONNECTED state.
> +
> + if (portType == OVS_VPORT_TYPE_INTERNAL) {
> + vport = gOvsSwitchContext->internalVport;
> + } else if (portType == OVS_VPORT_TYPE_NETDEV) {
> + if (!strcmp(portName, "external")) {
> + vport = gOvsSwitchContext->externalVport;
> + } else {
> + vport = OvsFindVportByHvName(gOvsSwitchContext, portName);
I don't think we need to maintain a HvName as well as OvsName and maintain separate tables. We can tackle this in a later commit. It seems redundant to maintain 2 names. If we maintain only one table, OvsFindVportByOvsName() will return success, and we just check if vport->portNo != NULL.
OvsFindVportByHvName() has conflicting definitions for parameter #2 - PCHAR v/s PSTR. Furthermore, the definition of 'OvsFindVportByHvName()' relies on the sizeof(name), which will really not yield the strlen() of name.
> + }
> + } else {
> + /* XXX: change when other tunneling ports are added */
> + ASSERT(portType == OVS_VPORT_TYPE_VXLAN);
> +
> + if (gOvsSwitchContext->vxlanVport) {
> + nlError = NL_ERROR_EXIST;
> + goto Cleanup;
> + }
> +
> + vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
> + if (vport == NULL) {
> + nlError = NL_ERROR_NOMEM;
> + goto Cleanup;
> + }
> +
> + vport->ovsState = OVS_STATE_PORT_CREATED;
> +
> + /*
> + * XXX: when we allow configuring the vxlan udp port, we should read
> + * this from vport->options instead!
> + */
> + nlError = OvsInitVxlanTunnel(vport, VXLAN_UDP_PORT);
> + if (nlError != NL_ERROR_SUCCESS) {
> + goto Cleanup;
> + }
I see that there's a call to OvsInitVportCommon() in OvsInitVxlanTunnel(). OvsInitVportCommon() is common code to all tunnels, and should be called from this function itself rather than deleting it to OvsInitVxlanTunnel() which is for vxlan specific stuff.
Can you add an ASSERT that vport->ovsState == OVS_STATE_CONNECTED? Also, pls. add a comment that OvsInitVxlanTunnel() inserts the vport into the portIdHashArray, so this function does not have to do that insertion.
> + }
> +
> + if (!vport) {
> + nlError = NL_ERROR_INVAL;
> + goto Cleanup;
> + }
> +
> + if (vport->portNo != OVS_DPPORT_NUMBER_INVALID) {
> + nlError = NL_ERROR_EXIST;
> + goto Cleanup;
> + }
If there's any attempt to add a second 'internal' or 'external' port with different name such as: 'internal.foo' or 'external.foo', we should return "NOT SUPPORTED".
> +
> + /* Fill the data in vport */
> + vport->ovsType = portType;
> +
> + if (vportAttrs[OVS_VPORT_ATTR_PORT_NO] != NULL) {
> + /*
> + * XXX: when we implement the limit for ovs port number to be
> + * MAXUINT16, we'll need to check the port number received from the
> + * userspace.
> + */
> + vport->portNo = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_PORT_NO]);
> + } else {
> + vport->portNo = OvsComputeVportNo(gOvsSwitchContext);
> + if (vport->portNo == OVS_DPPORT_NUMBER_INVALID) {
> + nlError = NL_ERROR_NOMEM;
> + goto Cleanup;
> + }
> + }
> +
> + /* The ovs port name must be uninitialized. */
> + ASSERT(vport->ovsName[0] == '\0');
> + ASSERT(portNameLen <= OVS_MAX_PORT_NAME_LENGTH);
> +
> + RtlCopyMemory(vport->ovsName, portName, portNameLen);
Again, 'ovsName' is the same as 'portFriendlyName'. I don't think we should duplicate data without good reason.
OVS_MAX_PORT_NAME_LENGTH should actually be 16.
> +
> + /* if we don't have options, then vport->portOptions will be NULL */
> + vport->portOptions = vportAttrs[OVS_VPORT_ATTR_OPTIONS];
> +
> + /*
> + * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath,
> + * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set,
> + * it means we have an array of pids, instead of a single pid.
> + * ATM we assume we have one pid only.
> + */
> + vport->upcallPid = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_UPCALL_PID]);
> +
> + if (vport->ovsType == OVS_VPORT_TYPE_VXLAN) {
> + gOvsSwitchContext->vxlanVport = vport;
> + } else if (vport->ovsType == OVS_VPORT_TYPE_INTERNAL) {
> + gOvsSwitchContext->internalVport = vport;
> + gOvsSwitchContext->internalPortId = vport->portId;
This is redundant code. 'vport' was initialized earlier with 'gOvsSwitchContext->internalVport'. Similar for externalVport.
> + } else if (vport->ovsType == OVS_VPORT_TYPE_NETDEV &&
> + vport->isExternal) {
> + gOvsSwitchContext->externalVport = vport;
> + gOvsSwitchContext->externalPortId = vport->portId;
> + }
> +
> + /*
> + * insert the port into the hash array of ports: by port number and ovs
> + * and ovs (datapath) port name.
> + * NOTE: OvsJhashWords has portNo as "1" word. This is ok, because the
> + * portNo is stored in 2 bytes only (max port number = MAXUINT16).
> + */
> + hash = OvsJhashWords(&vport->portNo, 1, OVS_HASH_BASIS);
> + InsertHeadList(&gOvsSwitchContext->portNoHashArray[hash & OVS_VPORT_MASK],
> + &vport->portNoLink);
> +
> + hash = OvsJhashBytes(vport->ovsName, portNameLen, OVS_HASH_BASIS);
> + InsertHeadList(&gOvsSwitchContext->ovsPortNameHashArray[hash & OVS_VPORT_MASK],
> + &vport->ovsNameLink);
> +
> + status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer,
> + usrParamsCtx->outputLength,
> + gOvsSwitchContext->dpNo);
> +
> + *replyLen = msgOut->nlMsg.nlmsgLen;
Pls. check for status == STATUS_SUCCESS and then assign positive value to *replyLen.
> +
> +Cleanup:
> + NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
> +
> + if (nlError != NL_ERROR_SUCCESS) {
> + POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> + usrParamsCtx->outputBuffer;
> +
> + if (vport && vport->ovsType == OVS_VPORT_TYPE_VXLAN) {
> + OvsRemoveAndDeleteVport(gOvsSwitchContext, vport);
> + }
> +
> + BuildErrorMsg(msgIn, msgError, nlError);
> + *replyLen = msgError->nlMsg.nlmsgLen;
> + }
> +
> + return STATUS_SUCCESS;
> +}
> +
> /*
> * --------------------------------------------------------------------------
> * Utility function to map the output buffer in an IRP. The buffer is assumed
> diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
> index 5de17fc..7cee140 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -49,7 +49,6 @@
> extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
> extern PNDIS_SPIN_LOCK gOvsCtrlLock;
>
> -static POVS_VPORT_ENTRY OvsAllocateVport(VOID);
> static VOID OvsInitVportWithPortParam(POVS_VPORT_ENTRY vport,
> PNDIS_SWITCH_PORT_PARAMETERS portParam);
> static VOID OvsInitVportWithNicParam(POVS_SWITCH_CONTEXT switchContext,
> @@ -58,10 +57,6 @@ static VOID OvsInitPhysNicVport(POVS_VPORT_ENTRY vport, POVS_VPORT_ENTRY
> virtVport, UINT32 nicIndex);
> static VOID OvsInitPhysNicVport(POVS_VPORT_ENTRY vport, POVS_VPORT_ENTRY
> virtVport, UINT32 nicIndex);
> -static NDIS_STATUS OvsInitVportCommon(POVS_SWITCH_CONTEXT switchContext,
> - POVS_VPORT_ENTRY vport);
> -static VOID OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext,
> - POVS_VPORT_ENTRY vport);
We don't need these changes if we implement the new functions in Vport.c, and we should. We should eventually move the OvsGetVportCmdHandler() and friends to Vport.c as well.
> static __inline VOID OvsWaitActivate(POVS_SWITCH_CONTEXT switchContext,
> ULONG sleepMicroSec);
>
> @@ -81,15 +76,17 @@ HvOnCreatePort(POVS_SWITCH_CONTEXT switchContext,
> NdisAcquireRWLockWrite(switchContext->dispatchLock, &lockState, 0);
> vport = OvsFindVportByPortIdAndNicIndex(switchContext,
> portParam->PortId, 0);
> - if (vport != NULL) {
> + if (vport != NULL && !vport->hvDeleted) {
> status = STATUS_DATA_NOT_ACCEPTED;
> goto create_port_done;
> + } else if (!vport) {
> + vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
> + if (vport == NULL) {
> + status = NDIS_STATUS_RESOURCES;
> + goto create_port_done;
> + }
It is not sufficient to check if there's a vport with portId and NicIndex that match. We should also check on a valid name i.e. portFriendlyName.
1. portFriendlyName should be unique
2. portFriendlyName should be less than 16 bytes long.
> }
> - vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
> - if (vport == NULL) {
> - status = NDIS_STATUS_RESOURCES;
> - goto create_port_done;
> - }
> +
> OvsInitVportWithPortParam(vport, portParam);
> OvsInitVportCommon(switchContext, vport);
>
> @@ -463,23 +460,25 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT switchContext,
> return NULL;
> }
>
> -
> POVS_VPORT_ENTRY
> OvsFindVportByOvsName(POVS_SWITCH_CONTEXT switchContext,
> - CHAR *name,
> - UINT32 length)
> + PSTR name)
> {
> POVS_VPORT_ENTRY vport;
> PLIST_ENTRY head, link;
> - UINT32 hash = OvsJhashBytes((const VOID *)name, length, OVS_HASH_BASIS);
> + UINT32 hash;
> + SIZE_T length = strlen(name) + 1;
> +
> + hash = OvsJhashBytes((const VOID *)name, length, OVS_HASH_BASIS);
> head = &(switchContext->ovsPortNameHashArray[hash & OVS_VPORT_MASK]);
> +
> LIST_FORALL(head, link) {
> vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, ovsNameLink);
> - if (vport->ovsNameLen == length &&
> - RtlEqualMemory(name, vport->ovsName, length)) {
> + if (!strcmp(name, vport->ovsName)) {
> return vport;
> }
> }
> +
> return NULL;
> }
>
> @@ -549,7 +548,7 @@ OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT switchContext,
> }
> }
>
> -static POVS_VPORT_ENTRY
> +POVS_VPORT_ENTRY
> OvsAllocateVport(VOID)
> {
> POVS_VPORT_ENTRY vport;
> @@ -559,6 +558,7 @@ OvsAllocateVport(VOID)
> }
> RtlZeroMemory(vport, sizeof (OVS_VPORT_ENTRY));
> vport->ovsState = OVS_STATE_UNKNOWN;
> + vport->hvDeleted = FALSE;
> vport->portNo = OVS_DPPORT_NUMBER_INVALID;
>
> InitializeListHead(&vport->ovsNameLink);
> @@ -679,7 +679,7 @@ OvsInitPhysNicVport(POVS_VPORT_ENTRY vport,
>
> vport->ovsState = OVS_STATE_PORT_CREATED;
> }
> -static NDIS_STATUS
> +NDIS_STATUS
> OvsInitVportCommon(POVS_SWITCH_CONTEXT switchContext,
> POVS_VPORT_ENTRY vport)
> {
> @@ -725,7 +725,7 @@ OvsInitVportCommon(POVS_SWITCH_CONTEXT switchContext,
> }
>
>
> -static VOID
> +VOID
> OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext,
> POVS_VPORT_ENTRY vport)
> {
> @@ -903,34 +903,6 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext)
> }
> }
>
> -NTSTATUS
> -OvsInitTunnelVport(POVS_VPORT_ENTRY vport,
> - POVS_VPORT_ADD_REQUEST addReq)
> -{
> - size_t len;
> - NTSTATUS status = STATUS_SUCCESS;
> -
> - vport->ovsType = addReq->type;
> - vport->ovsState = OVS_STATE_PORT_CREATED;
> - RtlCopyMemory(vport->ovsName, addReq->name, OVS_MAX_PORT_NAME_LENGTH);
> - vport->ovsName[OVS_MAX_PORT_NAME_LENGTH - 1] = 0;
> - StringCbLengthA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1, &len);
> - vport->ovsNameLen = (UINT32)len;
> - switch (addReq->type) {
> - case OVS_VPORT_TYPE_GRE:
> - break;
> - case OVS_VPORT_TYPE_GRE64:
> - break;
> - case OVS_VPORT_TYPE_VXLAN:
> - status = OvsInitVxlanTunnel(vport, addReq);
> - break;
> - default:
> - ASSERT(0);
> - }
> - return status;
> -}
> -
> -NTSTATUS
> OvsConvertIfCountedStrToAnsiStr(PIF_COUNTED_STRING wStr,
> CHAR *str,
> UINT16 maxStrLen)
> diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/Vport.h
> index 79b1c72..f4c6bfc 100644
> --- a/datapath-windows/ovsext/Vport.h
> +++ b/datapath-windows/ovsext/Vport.h
> @@ -86,8 +86,8 @@ typedef struct _OVS_VPORT_ENTRY {
> OVS_VPORT_ERR_STATS errStats;
> UINT32 portNo;
> UINT32 mtu;
> + /* ovsName is the ovs (datapath) port name - it is null terminated. */
> CHAR ovsName[OVS_MAX_PORT_NAME_LENGTH];
> - UINT32 ovsNameLen;
>
> PVOID priv;
> NDIS_SWITCH_PORT_ID portId;
> @@ -108,6 +108,8 @@ typedef struct _OVS_VPORT_ENTRY {
> GUID netCfgInstanceId;
> BOOLEAN isExternal;
> UINT32 upcallPid; /* netlink upcall port id */
> + PNL_ATTR portOptions;
> + BOOLEAN hvDeleted; /* is the hyper-v switch port deleted? */
> } OVS_VPORT_ENTRY, *POVS_VPORT_ENTRY;
>
> struct _OVS_SWITCH_CONTEXT;
> @@ -115,9 +117,11 @@ struct _OVS_SWITCH_CONTEXT;
> POVS_VPORT_ENTRY
> OvsFindVportByPortNo(struct _OVS_SWITCH_CONTEXT *switchContext,
> UINT32 portNo);
> +
> +/* "name" is null-terminated */
> POVS_VPORT_ENTRY
> -OvsFindVportByOvsName(struct _OVS_SWITCH_CONTEXT *switchContext,
> - CHAR *name, UINT32 length);
> +OvsFindVportByOvsName(POVS_SWITCH_CONTEXT switchContext,
> + PSTR name);
>
> POVS_VPORT_ENTRY
> OvsFindVportByHvName(POVS_SWITCH_CONTEXT switchContext, PCHAR name);
> @@ -167,5 +171,14 @@ OvsGetExternalMtu()
> {
> return ((POVS_VPORT_ENTRY) OvsGetExternalVport())->mtu;
> }
> +
> +VOID OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext,
> + POVS_VPORT_ENTRY vport);
> +
> +NDIS_STATUS OvsInitVportCommon(POVS_SWITCH_CONTEXT switchContext,
> + POVS_VPORT_ENTRY vport);
> +
> +POVS_VPORT_ENTRY OvsAllocateVport(VOID);
> +
>
> #endif /* __VPORT_H_ */
> diff --git a/datapath-windows/ovsext/Vxlan.c b/datapath-windows/ovsext/Vxlan.c
> index 7f94634..5c252f8 100644
> --- a/datapath-windows/ovsext/Vxlan.c
> +++ b/datapath-windows/ovsext/Vxlan.c
> @@ -27,6 +27,7 @@
> #include "Flow.h"
> #include "PacketParser.h"
> #include "Checksum.h"
> +#include "Vport.h"
>
> #pragma warning( push )
> #pragma warning( disable:4127 )
> @@ -50,30 +51,43 @@
> /* Move to a header file */
> extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
>
> -NTSTATUS
> +/*
> + * udpDestPort: the vxlan is set as payload to a udp frame. If the destination
> + * port of an udp frame is udpDestPort, we understand it to be vxlan.
> +*/
> +NL_ERROR
> OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport,
> - POVS_VPORT_ADD_REQUEST addReq)
> + UINT16 udpDestPort)
> {
> POVS_VXLAN_VPORT vxlanPort;
> - NTSTATUS status = STATUS_SUCCESS;
> -
> - ASSERT(addReq->type == OVS_VPORT_TYPE_VXLAN);
> + NTSTATUS status;
>
> vxlanPort = OvsAllocateMemory(sizeof (*vxlanPort));
> if (vxlanPort == NULL) {
> - status = STATUS_INSUFFICIENT_RESOURCES;
> - } else {
> - RtlZeroMemory(vxlanPort, sizeof (*vxlanPort));
> - vxlanPort->dstPort = addReq->dstPort;
> - /*
> - * since we are installing the WFP filter before the port is created
> - * We need to check if it is the same number
> - * XXX should be removed later
> - */
> - ASSERT(vxlanPort->dstPort == VXLAN_UDP_PORT);
> - vport->priv = (PVOID)vxlanPort;
> + return NL_ERROR_NOMEM;
> }
> - return status;
> +
> + RtlZeroMemory(vxlanPort, sizeof(*vxlanPort));
> + vxlanPort->dstPort = udpDestPort;
> + /*
> + * since we are installing the WFP filter before the port is created
> + * We need to check if it is the same number
> + * XXX should be removed later
> + */
> + ASSERT(vxlanPort->dstPort == VXLAN_UDP_PORT);
> + vport->priv = (PVOID)vxlanPort;
> +
> + status = OvsInitVportCommon(gOvsSwitchContext, vport);
> + ASSERT(status == NDIS_STATUS_SUCCESS);
> +
> + vport->ovsState = OVS_STATE_CONNECTED;
> + vport->nicState = NdisSwitchNicStateConnected;
> + /*
> + * allow the vport to be deleted, because there is no hyper-v switch part
> + */
> + vport->hvDeleted = TRUE;
> +
> + return NL_ERROR_SUCCESS;
> }
OvsInitVxlanTunnel() returning NL_ERROR looks a little odd.
>
>
> diff --git a/datapath-windows/ovsext/Vxlan.h b/datapath-windows/ovsext/Vxlan.h
> index e777933..dab5d9b 100644
> --- a/datapath-windows/ovsext/Vxlan.h
> +++ b/datapath-windows/ovsext/Vxlan.h
> @@ -47,8 +47,8 @@ typedef struct VXLANHdr {
> UINT32 reserved2:8;
> } VXLANHdr;
>
> -NTSTATUS OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport,
> - POVS_VPORT_ADD_REQUEST addReq);
> +NL_ERROR OvsInitVxlanTunnel(POVS_VPORT_ENTRY vport,
> + UINT16 udpDestPort);
> VOID OvsCleanupVxlanTunnel(POVS_VPORT_ENTRY vport);
>
> --
> 1.8.3.msysgit.0
>
Thanks,
-- Nithin
More information about the dev
mailing list