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

Message ID 1441152081-15202-1-git-send-email-svinturis@cloudbasesolutions.com
State Changes Requested
Headers show

Commit Message

Sorin Vinturis Sept. 18, 2015, 9:31 a.m. UTC
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(-)

Comments

Nithin Raju Sept. 18, 2015, 7:18 p.m. UTC | #1
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=
Sorin Vinturis Sept. 22, 2015, 5:40 a.m. UTC | #2
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=
Nithin Raju Sept. 22, 2015, 2:51 p.m. UTC | #3
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=

>

Patch
diff mbox

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,