| Message ID | 1441152081-15202-1-git-send-email-svinturis@cloudbasesolutions.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <svinturis@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 <svinturis@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(). > 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_mailman_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=PtJvYDEqho5jh2hAQ0i3eGD0u2IEwwDQ3votDGo-nx4&s=-u9SRUnalZagEUwblelvvVt2X7a-bmgd4XxbKplH_Ac&e=
Hi Nithin, Please see my answer inline. Thanks, Sorin -----Original Message----- From: Nithin Raju [mailto:nithin@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 <svinturis@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 <svinturis@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=
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 <nithin@vmare.com> > On Sep 21, 2015, at 10:40 PM, Sorin Vinturis <svinturis@cloudbasesolutions.com> wrote: > > Hi Nithin, > > Please see my answer inline. > > Thanks, > Sorin > > -----Original Message----- > From: Nithin Raju [mailto:nithin@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 <svinturis@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 <svinturis@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= >
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); 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,
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 <svinturis@cloudbasesolutions.com> --- datapath-windows/ovsext/Vport.c | 85 +++++++++++++++++++---------------------- datapath-windows/ovsext/Vport.h | 5 --- 2 files changed, 40 insertions(+), 50 deletions(-)