Sound good. Thanks for the explanation. If you don’t mind, can you pls. add a comment to this effect? The code in Vport.c is kind of complex, and comments would really help.
Acked-by: Nithin Raju <nit...@vmare.com> > On Sep 21, 2015, at 10:40 PM, Sorin Vinturis > <svintu...@cloudbasesolutions.com> wrote: > > Hi Nithin, > > Please see my answer inline. > > Thanks, > Sorin > > -----Original Message----- > From: Nithin Raju [mailto:nit...@vmware.com] > Sent: Friday, 18 September, 2015 22:19 > To: Sorin Vinturis > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3] datapath-windows: Removed hardcoded names > for internal/external vports > > hi Sorin, > The change looks good overall. I have one comment in > OvsInitConfiguredSwitchNics() which I had asked in the review for the v1 > patch as well. Not sure if you got a chance to address that. > > thanks, > -- Nithin > > >> On Sep 18, 2015, at 2:31 AM, Sorin Vinturis >> <svintu...@cloudbasesolutions.com> wrote: >> >> The internal/external vports will have the actual OS-based names, >> which represent the NIC interface alias that is displayed by running >> 'Get-NetAdapter' Hyper-V PS command. >> >> Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> >> --- >> datapath-windows/ovsext/Vport.c | 85 >> +++++++++++++++++++---------------------- >> datapath-windows/ovsext/Vport.h | 5 --- >> 2 files changed, 40 insertions(+), 50 deletions(-) >> >> diff --git a/datapath-windows/ovsext/Vport.c >> b/datapath-windows/ovsext/Vport.c index ea10692..b1a1e01 100644 >> --- a/datapath-windows/ovsext/Vport.c >> +++ b/datapath-windows/ovsext/Vport.c >> @@ -95,7 +95,7 @@ static VOID OvsTunnelVportPendingInit(PVOID context, >> static VOID OvsTunnelVportPendingRemove(PVOID context, >> NTSTATUS status, >> UINT32 *replyLen); >> - >> +static VOID AssignNicNameSpecial(POVS_VPORT_ENTRY vport); >> >> /* >> * Functions implemented in relaton to NDIS port manipulation. >> @@ -191,10 +191,8 @@ HvUpdatePort(POVS_SWITCH_CONTEXT switchContext, >> portParam->PortId, 0); >> /* >> * Update properties only for NETDEV ports for supprting PS script >> - * We don't allow changing the names of the internal or external ports >> */ >> - if (vport == NULL || (( vport->portType != NdisSwitchPortTypeSynthetic) >> && >> - ( vport->portType != NdisSwitchPortTypeEmulated))) { >> + if (vport == NULL) { >> goto update_port_done; >> } >> >> @@ -439,6 +437,7 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext, >> case NdisSwitchNicTypeInternal: >> RtlCopyMemory(&vport->netCfgInstanceId, &nicParam->NetCfgInstanceId, >> sizeof (GUID)); >> + AssignNicNameSpecial(vport); >> break; >> case NdisSwitchNicTypeSynthetic: >> case NdisSwitchNicTypeEmulated: >> @@ -968,36 +967,47 @@ OvsInitBridgeInternalVport(POVS_VPORT_ENTRY >> vport) >> >> /* >> * >> ---------------------------------------------------------------------- >> ---- >> - * For external vports 'portFriendlyName' provided by Hyper-V is >> over-written >> - * by synthetic names. >> + * For external and internal vports 'portFriendlyName' parameter, >> + provided by >> + * Hyper-V, is overwritten with the interface alias name. >> * >> ---------------------------------------------------------------------- >> ---- >> */ >> static VOID >> AssignNicNameSpecial(POVS_VPORT_ENTRY vport) { >> - size_t len; >> + NTSTATUS status = STATUS_SUCCESS; >> + WCHAR interfaceName[IF_MAX_STRING_SIZE] = { 0 }; >> + NET_LUID interfaceLuid = { 0 }; >> + size_t len = 0; >> >> - if (vport->portType == NdisSwitchPortTypeExternal) { >> - if (vport->nicIndex == 0) { >> - ASSERT(vport->nicIndex == 0); >> - RtlStringCbPrintfW(vport->portFriendlyName.String, >> - IF_MAX_STRING_SIZE, >> - L"%s.virtualAdapter", >> OVS_DPPORT_EXTERNAL_NAME_W); >> + ASSERT(vport->portType == NdisSwitchPortTypeExternal || >> + vport->portType == NdisSwitchPortTypeInternal); >> + >> + status = ConvertInterfaceGuidToLuid(&vport->netCfgInstanceId, >> + &interfaceLuid); >> + if (status == STATUS_SUCCESS) { >> + status = ConvertInterfaceLuidToAlias(&interfaceLuid, interfaceName, >> + IF_MAX_STRING_SIZE + 1); >> + if (status == STATUS_SUCCESS) { >> + if (vport->portType == NdisSwitchPortTypeExternal && >> + vport->nicIndex == 0) { >> + RtlStringCbPrintfW(vport->portFriendlyName.String, >> IF_MAX_STRING_SIZE, >> + L"%s.virtualAdapter", interfaceName); >> + } else { >> + RtlStringCbPrintfW(vport->portFriendlyName.String, >> + IF_MAX_STRING_SIZE, L"%s", >> interfaceName); >> + } >> + >> + RtlStringCbLengthW(vport->portFriendlyName.String, >> IF_MAX_STRING_SIZE, >> + &len); >> + vport->portFriendlyName.Length = (USHORT)len; >> } else { >> - RtlStringCbPrintfW(vport->portFriendlyName.String, >> - IF_MAX_STRING_SIZE, >> - L"%s.%lu", OVS_DPPORT_EXTERNAL_NAME_W, >> - (UINT32)vport->nicIndex); >> + OVS_LOG_INFO("Fail to convert interface LUID to alias, status: >> %x", >> + status); >> } >> } else { >> - RtlStringCbPrintfW(vport->portFriendlyName.String, >> - IF_MAX_STRING_SIZE, >> - L"%s", OVS_DPPORT_INTERNAL_NAME_W); >> + OVS_LOG_INFO("Fail to convert interface GUID to LUID, status: %x", >> + status); >> } >> - >> - RtlStringCbLengthW(vport->portFriendlyName.String, IF_MAX_STRING_SIZE, >> - &len); >> - vport->portFriendlyName.Length = (USHORT)len; >> } >> >> >> @@ -1382,6 +1392,7 @@ OvsInitConfiguredSwitchNics(POVS_SWITCH_CONTEXT >> switchContext) >> if (vport) { >> OvsInitPhysNicVport(vport, virtExtVport, >> nicParam->NicIndex); >> + OvsInitVportWithNicParam(switchContext, vport, >> + nicParam); >> status = InitHvVportCommon(switchContext, vport, TRUE); >> if (status != NDIS_STATUS_SUCCESS) { >> OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG); >> @@ -1392,13 +1403,15 @@ OvsInitConfiguredSwitchNics(POVS_SWITCH_CONTEXT >> switchContext) >> vport = OvsFindVportByPortIdAndNicIndex(switchContext, >> nicParam->PortId, >> >> nicParam->NicIndex); >> + OvsInitVportWithNicParam(switchContext, vport, nicParam); >> } >> if (vport == NULL) { >> OVS_LOG_ERROR("Fail to allocate vport"); >> continue; >> } >> - OvsInitVportWithNicParam(switchContext, vport, nicParam); >> if (nicParam->NicType == NdisSwitchNicTypeInternal) { >> + /* Overwrite the 'portFriendlyName' of the internal vport. */ >> + AssignNicNameSpecial(vport); > > Why do we need this call to AssignNicNameSpecial()? I asked this question in > the previous review as well. > > Like I mentioned, we’d need to allocate a new vport, and assign name only for > external NICs (ie. index > 0). For internal NICs, we’d have already assigned > the special name in OvsAddConfiguredSwitchPorts() -> InitHvVportCommon(). > > SV: Indeed the AssignNicNameSpecial() for the internal port is called when > the port is created in OvsAddConfiguredSwitchPorts() -> InitHvVportCommon(), > but, at that time, the vport does not have a netcfgId which is needed to > obtain the network alias. Thus the AssignNicNameSpecial() is not successful > and that is why the above call is added. > > >> OvsInternalAdapterUp(vport->portNo, &nicParam->NetCfgInstanceId); >> } >> } >> @@ -2093,9 +2106,7 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT >> usrParamsCtx, >> PCHAR portName; >> ULONG portNameLen; >> UINT32 portType; >> - BOOLEAN isBridgeInternal = FALSE; >> BOOLEAN vportAllocated = FALSE, vportInitialized = FALSE; >> - BOOLEAN addInternalPortAsNetdev = FALSE; >> >> static const NL_POLICY ovsVportPolicy[] = { >> [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = >> TRUE }, @@ -2138,24 +2149,12 @@ >> OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, >> goto Cleanup; >> } >> >> - if (portName && portType == OVS_VPORT_TYPE_NETDEV && >> - !strcmp(OVS_DPPORT_INTERNAL_NAME_A, portName)) { >> - addInternalPortAsNetdev = TRUE; >> - } >> - >> - if (portName && portType == OVS_VPORT_TYPE_INTERNAL && >> - strcmp(OVS_DPPORT_INTERNAL_NAME_A, portName)) { >> - isBridgeInternal = TRUE; >> - } >> - >> - if (portType == OVS_VPORT_TYPE_INTERNAL && !isBridgeInternal) { >> - vport = gOvsSwitchContext->internalVport; >> - } else if (portType == OVS_VPORT_TYPE_NETDEV) { >> + if (portType == OVS_VPORT_TYPE_NETDEV) { >> /* External ports can also be looked up like VIF ports. */ >> vport = OvsFindVportByHvNameA(gOvsSwitchContext, portName); >> } else { >> ASSERT(OvsIsTunnelVportType(portType) || >> - (portType == OVS_VPORT_TYPE_INTERNAL && isBridgeInternal)); >> + portType == OVS_VPORT_TYPE_INTERNAL); >> >> vport = (POVS_VPORT_ENTRY)OvsAllocateVport(); >> if (vport == NULL) { >> @@ -2221,10 +2220,6 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT >> usrParamsCtx, >> goto Cleanup; >> } >> >> - /* Initialize the vport with OVS specific properties. */ >> - if (addInternalPortAsNetdev != TRUE) { >> - vport->ovsType = portType; >> - } >> if (vportAttrs[OVS_VPORT_ATTR_PORT_NO] != NULL) { >> /* >> * XXX: when we implement the limit for ovs port number to be >> diff --git a/datapath-windows/ovsext/Vport.h >> b/datapath-windows/ovsext/Vport.h index ba21c62..ead55a9 100644 >> --- a/datapath-windows/ovsext/Vport.h >> +++ b/datapath-windows/ovsext/Vport.h >> @@ -34,11 +34,6 @@ >> */ >> #define OVS_DPPORT_NUMBER_LOCAL 0 >> >> -#define OVS_DPPORT_INTERNAL_NAME_A "internal" >> -#define OVS_DPPORT_INTERNAL_NAME_W L"internal" >> -#define OVS_DPPORT_EXTERNAL_NAME_A "external" >> -#define OVS_DPPORT_EXTERNAL_NAME_W L"external" >> - >> /* >> * A Vport, or Virtual Port, is a port on the OVS. It can be one of >> the >> * following types. Some of the Vports are "real" ports on the hyper-v >> switch, >> -- >> 1.9.0.msysgit.0 >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_ma >> ilman_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- >> uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=PtJvYDEqho5jh2hAQ0 >> i3eGD0u2IEwwDQ3votDGo-nx4&s=-u9SRUnalZagEUwblelvvVt2X7a-bmgd4XxbKplH_A >> c&e= > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
