I missed a couple of comments. Here are my comments: >- OvsCopyPortParamsFromVport(virtExtVport, &portParam); >+ char convertString[256]; >+ RtlZeroMemory(convertString, 256); > NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
Sai - Do you need to release this lock here for OvsConvertIfCountedStrToAnsiStr? This lock again gets acquired afterwards. Alin: We need to release the lock to get back to passive level. OvsConvertIfCountedStrToAnsiStr calls RtlUnicodeStringToAnsiString(https://msdn.microsoft.com/en-us/library/windows/hardware/ff562969(v=vs.85).aspx ) which needs to be called at passive level, if we still have the lock we are at dispatch >- status = HvCreatePort(switchContext, &portParam, >- nicParam->NicIndex); >+ status = OvsConvertIfCountedStrToAnsiStr(&portFriendlyName, >+ convertString, >+ >OVS_MAX_PORT_NAME_LENGTH); > NdisAcquireRWLockWrite(switchContext->dispatchLock, &lockState, > if (status != NDIS_STATUS_SUCCESS) { >- goto add_nic_done; Sai - switchContext->dispatchLock needs to be released here. Alin: rebase issue, it should go to add_nic_done; >+ goto done; >+ } >+ POVS_VPORT_ENTRY ovsVport = OvsFindVportByOvsName(switchContext, >+ convertString); >+ if (ovsVport != NULL) { >+ UpdateSwitchCtxWithVport(switchContext, ovsVport, FALSE); >+ } else { >+ NDIS_SWITCH_PORT_PARAMETERS portParam; >+ POVS_VPORT_ENTRY virtExtVport = >+ (POVS_VPORT_ENTRY)switchContext->virtualExternalVport; >+ >+ ASSERT(virtExtVport); >+ OvsCopyPortParamsFromVport(virtExtVport, &portParam); >+ NdisReleaseRWLock(switchContext->dispatchLock, &lockState); >+ status = HvCreatePort(switchContext, &portParam, >+ nicParam->NicIndex); >+ NdisAcquireRWLockWrite(switchContext->dispatchLock, >&lockState, 0); >+ if (status != NDIS_STATUS_SUCCESS) { Sai - switchContext->dispatchLock needs to be released here. >+ goto done; Alin: rebase issue, it should go to add_nic_done; >+ } >+ if (hvDelete && vport->isAbsentOnHv == FALSE) { >+ switchContext->internalPortId = 0; Alin: this is not needed removed in next version. >+ switchContext->countInternalVports--; Sai - Is this ASSERT really necessary? Alin - yes, the assert is needed for roleover >+ ASSERT(switchContext->countInternalVports >= 0); >+ OvsInternalAdapterDown(vport->portNo, >vport->netCfgInstanceId); > -----Original Message----- > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Alin > Serdean > Sent: Saturday, August 13, 2016 12:24 AM > To: Sairam Venugopal <vsai...@vmware.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3 2/3] datapath-windows: Add multiple > switch internal ports > > Hi Sai, > > Thanks a lot for the review! > > Comments inlined. > > Alin. > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev