[ovs-dev,6/6,v3] datapath-windows: Fix HvUpdateNic() to handle name changes

If the name of an internal or external NIC changes, we need to
disconnect the NIC from OVS since the name is the key. In this
change, we generate a link down event. It is as though we got a
call to HvDisconnectNic() for the old name and got a HvConnectNic()
for the new name.

Also, HvCreateNic() has been cleaned up to remove the code to look
for existing vport. We won't have a vport now since we'd have deleted
the vport in HvDeleteNic().

Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Sairam Venugopal <vsairam@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>

---
 datapath-windows/ovsext/Vport.c | 100 ++++++++++++++++++++--------------------
 datapath-windows/ovsext/Vport.h |  34 ++++++++++++--
 2 files changed, 80 insertions(+), 54 deletions(-)

On 25 November 2015 at 12:00, Nithin Raju <nithin@vmware.com> wrote:

> If the name of an internal or external NIC changes, we need to
> disconnect the NIC from OVS since the name is the key. In this
> change, we generate a link down event. It is as though we got a
> call to HvDisconnectNic() for the old name and got a HvConnectNic()
> for the new name.
>
> Also, HvCreateNic() has been cleaned up to remove the code to look
> for existing vport. We won't have a vport now since we'd have deleted
> the vport in HvDeleteNic().
>
> Signed-off-by: Nithin Raju <nithin@vmware.com>
> Acked-by: Sairam Venugopal <vsairam@vmware.com>
> Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
>

Series Applied, thanks!


>
> ---
>  datapath-windows/ovsext/Vport.c | 100
> ++++++++++++++++++++--------------------
>  datapath-windows/ovsext/Vport.h |  34 ++++++++++++--
>  2 files changed, 80 insertions(+), 54 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Vport.c
> b/datapath-windows/ovsext/Vport.c
> index ef21fca..a7576d3 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -327,9 +327,8 @@ HvCreateNic(POVS_SWITCH_CONTEXT switchContext,
>          goto done;
>      }
>
> -    if (nicParam->NicType == NdisSwitchNicTypeInternal ||
> -        (nicParam->NicType == NdisSwitchNicTypeExternal &&
> -         nicParam->NicIndex != 0)) {
> +    if (OvsIsInternalNIC(nicParam->NicType) ||
> +        OvsIsRealExternalNIC(nicParam->NicType, nicParam->NicIndex)) {
>          GetNICAlias(&nicParam->NetCfgInstanceId, &portFriendlyName);
>      }
>
> @@ -339,44 +338,22 @@ HvCreateNic(POVS_SWITCH_CONTEXT switchContext,
>       * structure for each such NIC, and each NIC inherits a lot of
> properties
>       * from the parent external port.
>       */
> -    if (nicParam->NicType == NdisSwitchNicTypeExternal &&
> -        nicParam->NicIndex != 0) {
> +    if (OvsIsRealExternalNIC(nicParam->NicType, nicParam->NicIndex)) {
> +        NDIS_SWITCH_PORT_PARAMETERS portParam;
>          POVS_VPORT_ENTRY virtExtVport =
>              (POVS_VPORT_ENTRY)switchContext->virtualExternalVport;
>
> -        vport = OvsFindVportByPortIdAndNicIndex(switchContext,
> -                                                nicParam->PortId,
> -                                                nicParam->NicIndex);
> -        if (vport == NULL) {
> -            NDIS_SWITCH_PORT_PARAMETERS portParam;
> -            /* Find by interface name */
> -            WCHAR interfaceName[IF_MAX_STRING_SIZE] = { 0 };
> -            NET_LUID interfaceLuid = { 0 };
> -            size_t len = 0;
> -            status =
> ConvertInterfaceGuidToLuid(&nicParam->NetCfgInstanceId,
> -                                                &interfaceLuid);
> -            if (status == STATUS_SUCCESS) {
> -                status = ConvertInterfaceLuidToAlias(&interfaceLuid,
> -                                                     interfaceName,
> -                                                     IF_MAX_STRING_SIZE +
> 1);
> -                if (status == STATUS_SUCCESS) {
> -                    RtlStringCbLengthW(interfaceName,
> -                                       IF_MAX_STRING_SIZE,
> -                                       &len);
> -                    vport = OvsFindVportByHvNameW(switchContext,
> -                                                  interfaceName,
> -                                                  len);
> -                }
> -            }
> -
> -            OvsCopyPortParamsFromVport(virtExtVport, &portParam);
> -            NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
> -            status = HvCreatePort(switchContext, &portParam,
> -                                  nicParam->NicIndex);
> -            NdisAcquireRWLockWrite(switchContext->dispatchLock,
> &lockState, 0);
> -            if (status != NDIS_STATUS_SUCCESS) {
> -                goto add_nic_done;
> -            }
> +        ASSERT(virtExtVport);
> +        ASSERT(OvsFindVportByPortIdAndNicIndex(switchContext,
> +                                               nicParam->PortId,
> +                                               nicParam->NicIndex) ==
> NULL);
> +        OvsCopyPortParamsFromVport(virtExtVport, &portParam);
> +        NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
> +        status = HvCreatePort(switchContext, &portParam,
> +                              nicParam->NicIndex);
> +        NdisAcquireRWLockWrite(switchContext->dispatchLock, &lockState,
> 0);
> +        if (status != NDIS_STATUS_SUCCESS) {
> +            goto add_nic_done;
>          }
>      }
>
> @@ -390,9 +367,8 @@ HvCreateNic(POVS_SWITCH_CONTEXT switchContext,
>          goto add_nic_done;
>      }
>      OvsInitVportWithNicParam(switchContext, vport, nicParam);
> -    if (nicParam->NicType == NdisSwitchNicTypeInternal ||
> -        (nicParam->NicType == NdisSwitchNicTypeExternal &&
> -         nicParam->NicIndex != 0)) {
> +    if (OvsIsInternalNIC(nicParam->NicType) ||
> +        OvsIsRealExternalNIC(nicParam->NicType, nicParam->NicIndex)) {
>          RtlCopyMemory(&vport->portFriendlyName, &portFriendlyName,
>                        sizeof portFriendlyName);
>      }
> @@ -470,6 +446,8 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext,
>      LOCK_STATE_EX lockState;
>      UINT32 event = 0;
>      IF_COUNTED_STRING portFriendlyName = {0};
> +    BOOLEAN nameChanged = FALSE;
> +    BOOLEAN aliasLookup = FALSE;
>
>      VPORT_NIC_ENTER(nicParam);
>
> @@ -483,9 +461,9 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext,
>
>      /* GetNICAlias() must be called outside of a lock. */
>      if (nicParam->NicType == NdisSwitchNicTypeInternal ||
> -        (nicParam->NicType == NdisSwitchNicTypeExternal &&
> -         nicParam->NicIndex != 0)) {
> +        OvsIsRealExternalNIC(nicParam->NicType, nicParam->NicIndex)) {
>          GetNICAlias(&nicParam->NetCfgInstanceId, &portFriendlyName);
> +        aliasLookup = TRUE;
>      }
>
>      NdisAcquireRWLockWrite(switchContext->dispatchLock, &lockState, 0);
> @@ -502,8 +480,15 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext,
>      case NdisSwitchNicTypeInternal:
>          RtlCopyMemory(&vport->netCfgInstanceId,
> &nicParam->NetCfgInstanceId,
>                        sizeof (GUID));
> -        RtlCopyMemory(&vport->portFriendlyName, &portFriendlyName,
> -                      sizeof portFriendlyName);
> +        if (aliasLookup) {
> +            if (RtlCompareMemory(&vport->portFriendlyName,
> +                    &portFriendlyName, vport->portFriendlyName.Length) !=
> +                    vport->portFriendlyName.Length) {
> +                RtlCopyMemory(&vport->portFriendlyName, &portFriendlyName,
> +                    sizeof portFriendlyName);
> +                nameChanged = TRUE;
> +            }
> +        }
>          break;
>      case NdisSwitchNicTypeSynthetic:
>      case NdisSwitchNicTypeEmulated:
> @@ -536,6 +521,17 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext,
>      }
>      vport->numaNodeId = nicParam->NumaNodeId;
>
> +    if (nameChanged) {
> +        OVS_EVENT_ENTRY event;
> +        event.portNo = vport->portNo;
> +        event.ovsType = vport->ovsType;
> +        event.upcallPid = vport->upcallPid;
> +        RtlCopyMemory(&event.ovsName, &vport->ovsName, sizeof
> event.ovsName);
> +        event.type = OVS_EVENT_LINK_DOWN;
> +        OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, TRUE);
> +        OvsPostEvent(&event);
> +    }
> +
>      NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
>
>      /*
> @@ -598,14 +594,15 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
>      RtlCopyMemory(&event.ovsName, &vport->ovsName, sizeof event.ovsName);
>      event.type = OVS_EVENT_LINK_DOWN;
>
> -    NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
> -
>      /*
>       * Delete the port from the hash tables accessible to userspace.
> After this
>       * point, userspace should not be able to access this port.
>       */
> -    OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, TRUE);
> -    OvsPostEvent(&event);
> +    if (OvsIsRealExternalVport(vport)) {
> +        OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, TRUE);
> +        OvsPostEvent(&event);
> +    }
> +    NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
>
>      if (isInternalPort) {
>          OvsInternalAdapterDown();
> @@ -650,8 +647,8 @@ HvDeleteNic(POVS_SWITCH_CONTEXT switchContext,
>      vport->nicState = NdisSwitchNicStateUnknown;
>      vport->ovsState = OVS_STATE_PORT_CREATED;
>
> -    if (vport->portType == NdisSwitchPortTypeExternal &&
> -        vport->nicIndex != 0) {
> +    if (OvsIsRealExternalVport(vport)) {
> +        /* This vport was created in HvCreateNic(). */
>          OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, FALSE);
>      }
>
> @@ -926,6 +923,7 @@ OvsInitVportWithNicParam(POVS_SWITCH_CONTEXT
> switchContext,
>      vport->mtu = nicParam->MTU;
>      vport->nicState = nicParam->NicState;
>      vport->nicIndex = nicParam->NicIndex;
> +    vport->nicType = nicParam->NicType;
>      vport->numaNodeId = nicParam->NumaNodeId;
>
>      switch (vport->nicState) {
> diff --git a/datapath-windows/ovsext/Vport.h
> b/datapath-windows/ovsext/Vport.h
> index 0d56484..e9f3b03 100644
> --- a/datapath-windows/ovsext/Vport.h
> +++ b/datapath-windows/ovsext/Vport.h
> @@ -95,6 +95,7 @@ typedef struct _OVS_VPORT_ENTRY {
>      PVOID                  priv;
>      NDIS_SWITCH_PORT_ID    portId;
>      NDIS_SWITCH_NIC_INDEX  nicIndex;
> +    NDIS_SWITCH_NIC_TYPE   nicType;
>      UINT16                 numaNodeId;
>      NDIS_SWITCH_PORT_STATE portState;
>      NDIS_SWITCH_NIC_STATE  nicState;
> @@ -194,14 +195,41 @@ OvsIsInternalVportType(OVS_VPORT_TYPE ovsType)
>  }
>
>  static __inline BOOLEAN
> +OvsIsVirtualExternalVport(POVS_VPORT_ENTRY vport)
> +{
> +    return vport->nicType == NdisSwitchNicTypeExternal &&
> +           vport->nicIndex == 0;
> +}
> +
> +static __inline BOOLEAN
> +OvsIsRealExternalVport(POVS_VPORT_ENTRY vport)
> +{
> +    return vport->nicType == NdisSwitchNicTypeExternal &&
> +           vport->nicIndex != 0;
> +}
> +
> +static __inline BOOLEAN
>  OvsIsBridgeInternalVport(POVS_VPORT_ENTRY vport)
>  {
> -    if (vport->isBridgeInternal) {
> -       ASSERT(vport->ovsType == OVS_VPORT_TYPE_INTERNAL);
> -    }
> +    ASSERT(vport->isBridgeInternal != TRUE ||
> +           vport->ovsType == OVS_VPORT_TYPE_INTERNAL);
>      return vport->isBridgeInternal == TRUE;
>  }
>
> +static __inline BOOLEAN
> +OvsIsInternalNIC(NDIS_SWITCH_NIC_TYPE   nicType)
> +{
> +    return nicType == NdisSwitchNicTypeInternal;
> +}
> +
> +static __inline BOOLEAN
> +OvsIsRealExternalNIC(NDIS_SWITCH_NIC_TYPE   nicType,
> +                     NDIS_SWITCH_NIC_INDEX  nicIndex)
> +{
> +    return nicType == NdisSwitchNicTypeExternal &&
> +           nicIndex != 0;
> +}
> +
>  NTSTATUS OvsRemoveAndDeleteVport(PVOID usrParamsCtx,
>                                   POVS_SWITCH_CONTEXT switchContext,
>                                   POVS_VPORT_ENTRY vport,
> --
> 1.8.5.6
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>