Hi Han Ding,
On Thu, Jan 05, 2023 at 02:15:03PM +0800, Han Ding wrote:
> Function is_gratuitous_arp() and function is_garp() are all used to judge
> whether the flow is gratuitous arp. It is not necessary to use two functions
> to do the same thing and just keep one.
I agree it is nice to consolidate things.
But, curiously, the logic of is_garp() and is_gratuitous_arp()
are somewhat different. I wonder about the risk of regressions
when consolidating them.
> Acked-by: Ilya Maximets<i.maxim...@ovn.org>
> Signed-off-by: Han Ding <hand...@chinatelecom.cn>
> ---
> lib/flow.h | 3 ++-
> ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
> 2 files changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/lib/flow.h b/lib/flow.h
> index c647ad83c..ade64b52d 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -1132,7 +1132,8 @@ static inline bool is_arp(const struct flow *flow)
> static inline bool is_garp(const struct flow *flow,
> struct flow_wildcards *wc)
> {
> - if (is_arp(flow)) {
> + if (is_arp(flow) &&
> + eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) {
Are any users relying on is_garp() not enforcing the
eth_addr_is_broadcast() check?
> return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
> FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
> }
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a9cf3cbee..b3c13f6bf 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct
> xbundle *out_xbundle,
> memcpy(&ctx->xin->flow.vlans, &old_vlans, sizeof(old_vlans));
> }
>
> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
> - * migration. Older Citrix-patched Linux DomU used gratuitous ARP replies to
> - * indicate this; newer upstream kernels use gratuitous ARP requests. */
> -static bool
> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
> -{
> - if (flow->dl_type != htons(ETH_TYPE_ARP)) {
> - return false;
> - }
> -
> - memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> - if (!eth_addr_is_broadcast(flow->dl_dst)) {
> - return false;
> - }
> -
> - memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
> - if (flow->nw_proto == ARP_OP_REPLY) {
> - return true;
This behaviour, which I assume correlates to the Citrix portion
of the comment above, does not seem to be present in is_garp().
Is that a problem?
> - } else if (flow->nw_proto == ARP_OP_REQUEST) {
> - memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
> - memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
> -
> - return flow->nw_src == flow->nw_dst;
> - } else {
This behaviour does not seem to be present in is_garp().
Is that a problem?
> - return false;
> - }
> -}
> -
> /* Determines whether packets in 'flow' within 'xbridge' should be forwarded
> or
> * dropped. Returns true if they may be forwarded, false if they should be
> * dropped.
...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
