Hi Sai, Thanks a lot for the review!
Comments inlined. Alin. > -----Original Message----- > From: Sairam Venugopal [mailto:vsai...@vmware.com] > Sent: Wednesday, August 10, 2016 1:16 AM > To: Alin Serdean <aserd...@cloudbasesolutions.com>; > dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3 2/3] datapath-windows: Add multiple > switch internal ports > > > > >- if (status == NDIS_STATUS_SUCCESS) { > > Sai: Could switchFwdInfo.vport be NULL is status == > NDIS_STATUS_SUCCESS? [Alin Serdean] Yes status can be SUCCESS and the switchFwdInfo.vport can be NULL. > > >+ if (status == NDIS_STATUS_SUCCESS && switchFwdInfo.vport != NULL) { > > ASSERT(newNbl); > >+ ovsFwdCtx->srcVportNo = switchFwdInfo.vport->portNo; > >+ ovsFwdCtx->fwdDetail->SourcePortId = switchFwdInfo.vport->portId; > >+ vsFwdCtx->fwdDetail->SourceNicIndex = > > Sai: Do we need the following function - OvsLookupFlowOutput? [Alin Serdean] Yes we still need it to rematch flows from internal ports. > > > OvsLookupFlowOutput(POVS_SWITCH_CONTEXT switchContext, > > */ > >- ASSERT(switchContext->internalVport); > > Sai: Shouldn¹t the Assert check for (countInternalVports > 0)? [Alin Serdean] Yes you are right. > > >+ ASSERT(switchContext->countInternalVports >= 0); > > status = OvsInitForwardingCtx(&ovsFwdCtx, switchContext, curNbl, > > internalVport->portNo, 0, > > > >+ ExAcquireResourceExclusiveLite(&instance->lock, TRUE); > >+ if (OvsCheckInstanceRow(&instance->internalRow, > >+ &ipRow->InterfaceLuid, > >+ ipRow->InterfaceIndex) > >+ ) { > > Sai: ix the alignment of these braces ({ [Alin Serdean] +1 > > >+ > > /* > > * Update the IP Interface Row > > */ > >- NdisAcquireSpinLock(&ovsIpHelperLock); > >- RtlCopyMemory(&ovsInternalIPRow, ipRow, > >- sizeof (PMIB_IPINTERFACE_ROW)); > >- ovsInternalIPConfigured = TRUE; > >- NdisReleaseSpinLock(&ovsIpHelperLock); > >+ RtlCopyMemory(&instance->internalIPRow, ipRow, > >+ sizeof(PMIB_IPINTERFACE_ROW)); > > Sai: Minor - fix alignment [Alin Serdean] +1 > > >OvsGetIPInterfaceEntry(instance->internalRow.InterfaceLuid, > >+ &instance->internalIPRow); > > > >- OvsCleanupFwdTable(); > >+ if (tatus == STATUS_SUCCESS) { > >+ instance->isIpConfigured = TRUE; > > } > > ai: Fix alignment [Alin Serdean] +1 > > >+ else { > >+ goto error; > >+ } > >+ > >+ status = OvsGetIPEntry(instance->internalRow.InterfaceLuid, > >&ipEntry); > >+ if (status != STATUS_SUCCESS) { > >+ OVS_LOG_INFO("Fail to get IP entry for internal port with > >GUID" > > Sai: Fix alignment [Alin Serdean] +1 > > > >+ ExAcquireResourceExclusiveLite(&instance->lock, TRUE); > >+ if (OvsCheckInsanceRow(&instance->internalRow, > >&ipRow->InterfaceLuid, > > Sai: This should be aligned with the other arguments [Alin Serdean] +1 > > >- if (ipRoute->InterfaceLuid.Info.NetLuidIndex == > >- ovsInternalRow.InterfaceLuid.Info.NetLuidIndex && > >- ipRoute->InterfaceLuid.Info.IfType == > >- ovsInternalRow.InterfaceLuid.Info.IfType && > >- ipRoute->InterfaceIndex == ovsInternalRow.InterfaceIndex) { > >+ ExAcquireResourceExclusiveLite(&instance->lock, TRUE); > >+ if (instance->isIpConfigured && > >+ OvsCheckInstanceRow(&instance->internalRow, > >+ &ipRoute->InterfaceLuid, > >+ ipRoute->InterfaceIndex) > > Sai: Fix the alignment for the braces [Alin Serdean] +1 > > >+ ExAcquireResourceExclusiveLite(&instance->lock, TRUE); > >+ if (instance->isIpConfigured && > >+ OvsCheckInstanceRow(&instance->internalRow, > >+ &unicastRow->InterfaceLuid, > >+ unicastRow->InterfaceIndex) > > Sai: Fix the alignment for the braces [Alin Serdean] +1 > > > >+ ExAcquireResourceExclusiveLite(&instance->lock, TRUE); > >+ if (instance->isIpConfigured && > >+ OvsCheckInstanceRow(&instance->internalRow, > >+ &unicastRow->InterfaceLuid, > >+ unicastRow->InterfaceIndex) > > Sai: Fix the alignment for the braces [Alin Serdean] +1 > > > >+ ) { > >+ > > static VOID > > OvsHandleInternalAdapterUp(POVS_IP_HELPER_REQUEST request) > > { > > Sai: request is referenced underneath. You can remove this line. [Alin Serdean] +1 > > >+ UNREFERENCED_PARAMETER(request); > >+ if (status != STATUS_SUCCESS) { > >+ OVS_LOG_ERROR("Fail to get IF entry for internal port with > >GUID" > > Sai: I feel we should add a function for printing GUID PrintGuid(logLevel, > char * msg, GUID guid) > This code is copied over too many times and might make the file look > cleaner. [Alin Serdean] I will look how can I improve it in the next version > > >+ " > >%08x-%04x-%04x-%04x-%02x%02x%02x%02x%02x%02x", > >+ instance->netCfgId.Data1, > >+ instance->netCfgId.Data2, > >+ instance->netCfgId.Data3, > >+ *(UINT16 *)instance->netCfgId.Data4, > >+ instance->netCfgId.Data4[2], > >+ instance->netCfgId.Data4[3], > >+ instance->netCfgId.Data4[4], > >+ instance->netCfgId.Data4[5], > >+ instance->netCfgId.Data4[6], > >+ instance->netCfgId.Data4[7]); > >+ break; > >+ } > > > >- if (status != STATUS_SUCCESS) { > >- OVS_LOG_ERROR("Fali to get IF entry for internal port with GUID" > > Sai: Fail not Fali [Alin Serdean] +1 > > >- " %08x-%04x-%04x-%04x-%02x%02x%02x%02x%02x%02x", > >- netCfgInstanceId->Data1, > >- netCfgInstanceId->Data2, > >- netCfgInstanceId->Data3, > >- *(UINT16 *)netCfgInstanceId->Data4, > >- netCfgInstanceId->Data4[2], > >- netCfgInstanceId->Data4[3], > >- netCfgInstanceId->Data4[4], > >- netCfgInstanceId->Data4[5], > >- netCfgInstanceId->Data4[6], > >- netCfgInstanceId->Data4[7]); > >- return; > >- } > >- if (status != STATUS_SUCCESS) { > >- OVS_LOG_INFO("Fali to get IP entry for internal port with GUID" > > Sai: Fail not Fali. Should probably use Failed* instead of Fail. [Alin Serdean] +1 > > > > OvsInitIpHelper(NDIS_HANDLE ndisFilterHandle) > > { > >- NTSTATUS status; > > Sai: ndisFilterHandle is referenced [Alin Serdean] will remove the line in the next version > > >+ UNREFERENCED_PARAMETER(ndisFilterHandle); > >+ NTSTATUS status = NDIS_STATUS_SUCCESS; > > if (OvsIsRealExternalVport(vport)) { > >- OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, > TRUE); > > Sai: Is this an existing bug? [Alin Serdean] Yep. This does not allow a physical port to be deleted from OVS. Eg: if you disable the network adapter on which the switch was created, you need to re-enable the extension so you can add the port back. I will remove it from the series and send it out in a different patch. > > >+ OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, > FALSE); > > OvsPostVportEvent(&event); > > } > > } > > break; > > case NdisSwitchPortTypeInternal: > >- ASSERT(vport->isBridgeInternal == FALSE); > > switchContext->internalPortId = vport->portId; > > Sai: Why do we still have (switchContext->internalPortId = vport->portId)? [Alin Serdean] Forgot to nuke it. I will delete the member in the next version > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev