[ovs-dev] [PATCH v3] datapath-windows: Removed hardcoded names for internal/external vports

Sorin Vinturis svinturis at cloudbasesolutions.com
Tue Sep 22 05:40:14 UTC 2015


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