hi Alin,
LG, but for the comments I had.
Acked-by: Nithin Raju <nit...@vmware.com>
On Sep 30, 2014, at 8:00 AM, Samuel Ghinet <sghi...@cloudbasesolutions.com>
wrote:
> Deletion of a vport is now handled both by the netlink command vport
> delete and the hyper-v switch port delete handler. If a netlink
> command vport delete is requested on a vport that has no hyper-v
> switch port counterpart (i.e., it is a tunnel port, or or the hyper-v
> switch virtual nic is disconnected), the vport is deleted and removed.
>
> If the hyper-v switch port delete is requested (i.e. the VNic is
> disconnecting) and the ovs (datapath) part is deleted (i.e. was
> deleted by netlink command vport delete, or was never created by
> an netlink command vport new), then the hyper-v switch port delete
> function handler deletes and removes the vport.
>
> If the hyper-v switch port delete is requested while its datapath
> counterpart is still alive, or, when the netlink command vport delete
> is requested while the hyper-v switch port is still alive, the port
> is only marked that it's part is deleted - the field hvDeleted was
> added to OVS_VPORT_ENTRY to specify if the hyper-v switch port side
> was deleted; if the ovs (datapath) port number is invalid, then it
> means that the ovs (datapath) side of the port is deleted (or, not
> created).
>
> Signed-off-by: Samuel Ghinet <sghi...@cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Datapath.c | 109 ++++++++++++++++++++++++++++++++++++-
> datapath-windows/ovsext/Vport.c | 23 +++++---
> 2 files changed, 123 insertions(+), 9 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index dc03961..7897330 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -104,7 +104,8 @@ static NetlinkCmdHandler OvsGetPidCmdHandler,
> OvsSetDpCmdHandler,
> OvsReadEventCmdHandler,
> OvsGetVportCmdHandler,
> - OvsNewVportCmdHandler;
> + OvsNewVportCmdHandler,
> + OvsDeleteVportCmdHandler;
>
> static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> UINT32 *replyLen);
> @@ -201,7 +202,12 @@ NETLINK_CMD nlVportFamilyCmdOps[] = {
> .handler = OvsNewVportCmdHandler,
> .supportedDevOp = OVS_TRANSACTION_DEV_OP,
> .validateDpIndex = TRUE
> - }
> + },
> + { .cmd = OVS_VPORT_CMD_DEL,
> + .handler = OvsDeleteVportCmdHandler,
> + .supportedDevOp = OVS_TRANSACTION_DEV_OP,
> + .validateDpIndex = TRUE
> + },
> };
>
> NETLINK_FAMILY nlVportFamilyOps = {
> @@ -1811,6 +1817,105 @@ Cleanup:
> return STATUS_SUCCESS;
> }
>
> +static NTSTATUS
> +OvsDeleteVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> + UINT32 *replyLen)
> +{
> + NDIS_STATUS status = STATUS_SUCCESS;
> + LOCK_STATE_EX lockState;
> +
> + POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
> + POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> + POVS_VPORT_ENTRY vport = NULL;
> + NL_ERROR nlError = NL_ERROR_SUCCESS;
> + PSTR portName = NULL;
> + UINT32 portNameLen = 0;
> +
> + static const NL_POLICY ovsVportPolicy[] = {
> + [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE },
> + [OVS_VPORT_ATTR_NAME] = { .type = NL_A_STRING, .maxLen = IFNAMSIZ,
> .optional = TRUE },
long line.
> + };
> + PNL_ATTR vportAttrs[ARRAY_SIZE(ovsVportPolicy)];
> +
> + ASSERT(usrParamsCtx->inputBuffer != NULL);
> +
> + if (!NlAttrParse((PNL_MSG_HDR)msgIn,
> + NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN,
> + ovsVportPolicy, vportAttrs, ARRAY_SIZE(vportAttrs))) {
> + return STATUS_INVALID_PARAMETER;
> + }
> +
> + if (msgOut == NULL || usrParamsCtx->outputLength < sizeof *msgOut) {
> + return STATUS_NDIS_INVALID_LENGTH;
> + }
> +
> + OvsAcquireCtrlLock();
> + if (!gOvsSwitchContext) {
> + OvsReleaseCtrlLock();
> + return STATUS_INVALID_PARAMETER;
> + }
> + OvsReleaseCtrlLock();
CtrlLock should be released after at the end of the function.
> +
> + NdisAcquireRWLockWrite(gOvsSwitchContext->dispatchLock, &lockState, 0);
> + if (vportAttrs[OVS_VPORT_ATTR_NAME] != NULL) {
> + 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]));
> + }
> +
> + if (!vport) {
> + nlError = NL_ERROR_NODEV;
> + goto Cleanup;
> + }
> +
> + status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer,
> + usrParamsCtx->outputLength,
> + gOvsSwitchContext->dpNo);
> +
> + if (vport->hvDeleted || OvsIsTunnelVportType(vport->ovsType)) {
> + /*
> + * The associated hyper-v switch port is not in created state, or,
> + * there is no hyper-v switch port counterpart (for logical ports).
> + * This means that this datapath port is not mapped to a living
> + * hyper-v switc hport. We can destroy and remove the vport from the
> + * list.
> + */
> + OvsRemoveAndDeleteVport(gOvsSwitchContext, vport);
> + } else {
> + /* The associated hyper-v switch port is in the created state, and
> the
> + * datapath port is mapped to a living hyper-v switch port. We cannot
> + * destroy the vport and cannot remove it from the list of vports.
> + * Instead, we mark the datapath (ovs) part of the vport as
> + * "not created", i.e. we set vport->portNo =
> OVS_PORT_NUMBER_INVALID.
> + */
> + vport->portNo = OVS_DPPORT_NUMBER_INVALID;
> + vport->ovsName[0] = '\0';
> + }
> +
> + *replyLen = msgOut->nlMsg.nlmsgLen;
Pls. set *replylen only on error.
> +
> +Cleanup:
> + NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
> +
> + if (nlError != NL_ERROR_SUCCESS) {
> + POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> + usrParamsCtx->outputBuffer;
> +
> + 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 7cee140..d90fd2b 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -121,28 +121,37 @@ HvOnTeardownPort(POVS_SWITCH_CONTEXT switchContext,
> VPORT_PORT_EXIT(portParam);
> }
>
> -
> -
> VOID
> HvOnDeletePort(POVS_SWITCH_CONTEXT switchContext,
> - PNDIS_SWITCH_PORT_PARAMETERS portParam)
> + PNDIS_SWITCH_PORT_PARAMETERS portParams)
> {
> POVS_VPORT_ENTRY vport;
> LOCK_STATE_EX lockState;
>
> - VPORT_PORT_ENTER(portParam);
> + VPORT_PORT_ENTER(portParams);
>
> NdisAcquireRWLockWrite(switchContext->dispatchLock, &lockState, 0);
> vport = OvsFindVportByPortIdAndNicIndex(switchContext,
> - portParam->PortId, 0);
> + portParams->PortId, 0);
> +
> + /*
> + * XXX: we can only destroy and remove the port if its datapath port
> + * counterpart was deleted. If the datapath port counterpart is present,
> + * we only mark the vport for deletion, so that a netlink command vport
> + * delete will delete the vport.
> + */
> if (vport) {
> - OvsRemoveAndDeleteVport(switchContext, vport);
Can we ASSERT here that vport->hvDeleted != TRUE.
> + if (vport->portNo == OVS_DPPORT_NUMBER_INVALID) {
> + OvsRemoveAndDeleteVport(switchContext, vport);
> + } else {
> + vport->hvDeleted = TRUE;
> + }
> } else {
We should ASSERT here, at least till the code stabilizes.
> OVS_LOG_WARN("Vport not present.");
> }
> NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
>
> - VPORT_PORT_EXIT(portParam);
> + VPORT_PORT_EXIT(portParams);
> }
>
>
> --
> 1.8.3.msysgit.0
>
Thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
