[ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

Amber Hu qhu at vmware.com
Wed Jan 22 06:56:26 UTC 2020


Hi Alin,

Will update the patch and change it according to the comments.
Thanks very much.

BR/Amber

On 1/22/20, 12:43, "Alin Serdean" <aserdean at cloudbasesolutions.com> wrote:

    Hi Amber,
    
    The patch fails to apply:
    "error: corrupt patch at line 198"
    
    Please rebase on master.
    
    Trimming the patch a bit and comments inlined.
    > -----Original Message-----
    > From: Amber Hu <qhu at vmware.com>
    > Sent: Tuesday, January 14, 2020 7:50 AM
    > To: dev at openvswitch.org; Alin Serdean <aserdean at cloudbasesolutions.com>
    > Cc: Qiong Wang <wqiong at vmware.com>; Wenyu Zhang
    > <wenyuz at vmware.com>; Jinjun Gao <jinjung at vmware.com>
    > Subject: [patch] datapath-windows: Append tunnel info to upcall for correct
    > template
    > 
    
    > diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
    > windows/ovsext/Actions.c
    > 
    > index 0c4d64b..936e68b 100644
    > 
    > --- a/datapath-windows/ovsext/Actions.c
    > 
    > +++ b/datapath-windows/ovsext/Actions.c
    > 
    > @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext
    > *ovsFwdCtx,
    > 
    >                           const PNL_ATTR attr)
    > 
    > {
    > 
    >      NTSTATUS status = NDIS_STATUS_SUCCESS;
    > 
    > -    PNL_ATTR userdataAttr;
    > 
    > -    PNL_ATTR queueAttr;
     [Alin Serdean] queueAttr can be safely removed since it's not actually used.
    > 
    > +    PNL_ATTR userdataAttr = NULL;
    > 
    > +    PNL_ATTR queueAttr = NULL;
    > 
    > +    PNL_ATTR egrTunAttr = NULL;
    > 
    > +    PNL_ATTR a = NULL;
    [Alin Serdean] Not sure why did is needed. NlAttrFindNested does the safe version
    of NL_ATTR_FOR_EACH_UNSAFE. Any reason to change it?
    > 
    >      POVS_PACKET_QUEUE_ELEM elem;
    > 
    >      POVS_PACKET_HDR_INFO layers = &ovsFwdCtx->layers;
    > 
    >      BOOLEAN isRecv = FALSE;
    > 
    > +    INT rem = 0;
    > 
    > +    OVS_FWD_INFO fwdInfo;
    > 
    > +    OvsIPv4TunnelKey tunKey;
    > 
    >      POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx-
    > >switchContext,
    > 
    >                                                    ovsFwdCtx->srcVportNo);
    > 
    > @@ -1855,13 +1860,41 @@ OvsOutputUserspaceAction(OvsForwardingContext
    > *ovsFwdCtx,
    > 
    >          }
    > 
    >      }
    > 
    > -    queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
    > 
    > -    userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
    > 
    > +    NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
    [Alin Serdean] NlAttrFindNested does the safe version of NL_ATTR_FOR_EACH_UNSAFE.
    Any reason to change it?
    > 
    > +        switch (NlAttrType(a)) {
    > 
    > +            case OVS_USERSPACE_ATTR_PID:
    > 
    > +                queueAttr = a;
    > 
    > +                break;
    > 
    > +            case OVS_USERSPACE_ATTR_USERDATA:
    > 
    > +                userdataAttr = a;
    > 
    > +                break;
    > 
    > +            case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
    > 
    > +                /* Indicate the packet is from egress tunnel port */
    > 
    > +                egrTunAttr = a;
    > 
    > +                break;
    > 
    > +            default:
    > 
    > +                break;
    > 
    > +        }
    > 
    > +    }
    > 
    > +
    > 
    > +    /* Fill tunnel key to export to usersspace to calculate the template id */
    > 
    > +    if (egrTunAttr) {
    > 
    [Alin Serdean] It would be safer to zero out tunKey before this.
    > +        RtlCopyMemory(&tunKey, &ovsFwdCtx->tunKey, sizeof tunKey);
    > 
    > +        if (!tunKey.src) {
    > 
    > +            status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, &fwdInfo);
    > 
    > +            if (status == NDIS_STATUS_SUCCESS && tunKey.dst ==
    > fwdInfo.dstIpAddr) {
    > 
    > +                tunKey.src = fwdInfo.srcIpAddr;
    > 
    > +            }
    > 
    > +        }
    > 
    > +        tunKey.flow_hash = tunKey.flow_hash?tunKey.flow_hash:MAXINT16;
    [Alin Serdean] Missing a whitespace before and after '?'
    > 
    > +    }
    > 
    >      elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
    > 
    >                                    NlAttrGetSize(userdataAttr),
    > 
    >                                    OVS_PACKET_CMD_ACTION,
    > 
    > -                                  vport, key, ovsFwdCtx->curNbl,
    > 
    > +                                  vport, key,
    > 
    > +                                  egrTunAttr?&(tunKey):NULL,
    [Alin Serdean] Missing whitespace before and after '?'
    > 
    > +                                  ovsFwdCtx->curNbl,
    > 
    >                                    NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl),
    > 
    >                                    isRecv,
    > 
    >                                    layers);
    > 
    > diff --git a/datapath-windows/ovsext/Flow.c b/datapath-
    > windows/ovsext/Flow.c
    > 
    > index fdb1010..ac0582c 100644
    > 
    > --- a/datapath-windows/ovsext/Flow.c
    > 
    > +++ b/datapath-windows/ovsext/Flow.c
    > 
    > @@ -1094,6 +1094,18 @@ MapFlowTunKeyToNlKey(PNL_BUFFER nlBuf,
    > 
    >          goto done;
    > 
    >      }
    > 
    > +    if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_SRC,
    > 
    > +                         tunKey->flow_hash)) {
    > 
    > +        rc = STATUS_UNSUCCESSFUL;
    > 
    > +        goto done;
    > 
    > +    }
    > 
    > +
    > 
    > +    if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_DST,
    > 
    > +                         tunKey->dst_port)) {
    > 
    > +        rc = STATUS_UNSUCCESSFUL;
    > 
    > +        goto done;
    > 
    > +    }
    > 
    > +
    > 
    > done:
    > 
    >      NlMsgEndNested(nlBuf, offset);
    > 
    > error_nested_start:
    > 
    > diff --git a/datapath-windows/ovsext/User.c b/datapath-
    > windows/ovsext/User.c
    > 
    > index 16d6fd2..a1d05e1 100644
    > 
    > --- a/datapath-windows/ovsext/User.c
    > 
    > +++ b/datapath-windows/ovsext/User.c
    > 
    > @@ -833,7 +833,7 @@ OvsCreateAndAddPackets(PVOID userData,
    > 
    >      nb = NET_BUFFER_LIST_FIRST_NB(nbl);
    > 
    >      while (nb) {
    > 
    >          elem = OvsCreateQueueNlPacket(userData, userDataLen,
    > 
    > -                                    cmd, vport, key, nbl, nb,
    > 
    > +                                    cmd, vport, key, NULL, nbl, nb,
    > 
    >                                      isRecv, hdrInfo);
    > 
    >          if (elem) {
    > 
    >              InsertTailList(list, &elem->link);
    > 
    > @@ -1016,6 +1016,7 @@ OvsCreateQueueNlPacket(PVOID userData,
    > 
    >                         UINT32 cmd,
    > 
    >                         POVS_VPORT_ENTRY vport,
    > 
    >                         OvsFlowKey *key,
    > 
    > +                       OvsIPv4TunnelKey *tunnelKey,
    > 
    >                         PNET_BUFFER_LIST nbl,
    > 
    >                         PNET_BUFFER nb,
    > 
    >                         BOOLEAN isRecv,
    > 
    > @@ -1028,7 +1029,6 @@ OvsCreateQueueNlPacket(PVOID userData,
    > 
    >      NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
    > 
    >      PNDIS_NET_BUFFER_LIST_8021Q_INFO vlanInfo = NULL;
    > 
    >      PVOID vlanTag;
    > 
    > -    OvsIPv4TunnelKey *tunnelKey = (OvsIPv4TunnelKey *)&key->tunKey;
    > 
    >      UINT32 pid;
    > 
    >      UINT32 nlMsgSize;
    > 
    >      NL_BUFFER nlBuf;
    > 
    > @@ -1131,6 +1131,12 @@ OvsCreateQueueNlPacket(PVOID userData,
    > 
    >      }
    > 
    [Alin Serdean] I guess this can be removed now 😊.
    >      /* XXX must send OVS_PACKET_ATTR_EGRESS_TUN_KEY if set by vswtchd */
    > 
    > +    if (tunnelKey) {
    > 
    > +        if (MapFlowTunKeyToNlKey(&nlBuf, tunnelKey,
    > 
    > +                                 OVS_PACKET_ATTR_EGRESS_TUN_KEY) != STATUS_SUCCESS)
    > {
    > 
    > +            goto fail;
    > 
    > +        }
    > 
    > +    }
    > 
    >      if (userData){
    > 
    >          if (!NlMsgPutTailUnspec(&nlBuf, OVS_PACKET_ATTR_USERDATA,
    > 
    >                                  userData, (UINT16)userDataLen)) {
    > 
    > diff --git a/datapath-windows/ovsext/User.h b/datapath-
    > windows/ovsext/User.h
    > 
    > index 3a42888..ccca0ba 100644
    > 
    > --- a/datapath-windows/ovsext/User.h
    > 
    > +++ b/datapath-windows/ovsext/User.h
    > 
    > @@ -75,6 +75,7 @@ POVS_PACKET_QUEUE_ELEM
    > OvsCreateQueueNlPacket(PVOID userData,
    > 
    >                                                UINT32 cmd,
    > 
    >                                                POVS_VPORT_ENTRY vport,
    > 
    >                                                OvsFlowKey *key,
    > 
    > +                                              OvsIPv4TunnelKey *tunnelKey,
    > 
    >                                                PNET_BUFFER_LIST nbl,
    > 
    >                                                PNET_BUFFER nb,
    > 
    >                                                BOOLEAN isRecv,
    > 
    > --
    > 
    > 2.6.2
    > 
    > 
    > 
    > BR/Amber
    
    



More information about the dev mailing list