[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