[ovs-dev] [PATCH v3 2/3] datapath-windows: Add multiple switch internal ports

[Alin Serdean]

Hi Sai,

Thanks a lot for the review!

Comments inlined.

Alin.

> -----Original Message-----
> From: Sairam Venugopal [mailto:vsairam at vmware.com]
> Sent: Wednesday, August 10, 2016 1:16 AM
> To: Alin Serdean <aserdean at cloudbasesolutions.com>;
> dev at 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
>