[ovs-dev] [PATCH v1] Add mirror/ovs-tcpdump support for slow protocols' Rx path

[Manohar Krishnappa Chidambaraswamy]

Hi Ben,

Thanx for the review. Please see inline.

On 08/05/18, 9:12 PM, "Ben Pfaff" <blp at ovn.org> wrote:

    On Tue, May 08, 2018 at 08:13:10AM +0000, Manohar Krishnappa Chidambaraswamy wrote:
    > Problem:
    > ========
    > Received LACP/CFM/BFD/STP/LLDP slow protocols' packets are not captured in
    > ovs-tcpdump.
    > 
    > Fix:
    > ====
    > Add mirror support for slow protocols.
    > 
    > Signed-off-by: Manohar K C
    > <manohar.krishnappa.chidambaraswamy at ericsson.com>
    > CC: Jan Scheurich <jan.scheurich at ericsson.com>
    
    Thanks for working on this.
    
    I see that there are two calls to process_special(): one in
    xlate_actions(), the other in patch_port_output().  Currently, it looks
    like patch_port_output() doesn't ever call mirror_ingress_packet().
    This might be a bug of its own, but adding a call to
    mirror_ingress_packet() only for the case when special processing is
    needed is an oddity.
    
    So, the following seems like a more consistent choice:
    
    diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
    index 5641724a7fa2..fe5fec77e5b4 100644
    --- a/ofproto/ofproto-dpif-xlate.c
    +++ b/ofproto/ofproto-dpif-xlate.c
    @@ -7211,6 +7211,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
              *
              * We do not perform special processing on thawed packets, since that
              * was done before they were frozen and should not be redone. */
    +        mirror_ingress_packet(&ctx);
         } else if (in_port && in_port->xbundle
                    && xbundle_mirror_out(xbridge, in_port->xbundle)) {
             xlate_report_error(&ctx, "dropping packet received on port "
    
    Separately, it might be a good idea to add proper ingress mirroring to
    patch_port_output().
    
    What do you think?
[manu] I agree. I will send v2 with this change to cover xlate_actions().
Also I think it would only be STP(among the ones handled in process_special())
traffic that takes patch_port_output() path.

Thanx
Manu
    
    Thanks,
    
    Ben.