[ovs-dev] [PATCH v3 2/3] datapath-windows: Add multiple switch internal ports
Alin Serdean
aserdean at cloudbasesolutions.com
Sat Aug 13 01:07:27 UTC 2016
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-bounces at openvswitch.org] On Behalf Of Alin
> Serdean
> Sent: Saturday, August 13, 2016 12:24 AM
> To: Sairam Venugopal <vsairam at vmware.com>; dev at 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.
>
More information about the dev
mailing list