[ovs-dev] [PATCH v3] datapath-windows: Removed hardcoded names for internal/external vports
Nithin Raju
nithin at vmware.com
Tue Sep 22 14:51:02 UTC 2015
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 at vmare.com>
> On Sep 21, 2015, at 10:40 PM, Sorin Vinturis <svinturis at cloudbasesolutions.com> wrote:
>
> Hi Nithin,
>
> Please see my answer inline.
>
> Thanks,
> Sorin
>
> -----Original Message-----
> From: Nithin Raju [mailto:nithin at vmware.com]
> Sent: Friday, 18 September, 2015 22:19
> To: Sorin Vinturis
> Cc: dev at 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 at 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 at 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 at 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=
>
More information about the dev
mailing list