Re: [PATCH v14 10/10] icmp: let icmp use mac address from flowside structure

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 14 Oct 2025 22:55:21 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> Even ICMP needs to be updated to use the external MAC address instead
> of just the own tap address when applicable. We do that here.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> ---
> v3: - Adapted to the move of external MAC address from struct flowside
>       to struct flow_common
> v4: - Adapted to name changes in previous commits in this series
> v5: - Added conditional lookup in ARP/NDP if the flow's tap_omac is
>       undefined
> v6: - Looking up MAC of ICMP generating node in udp_send_tap_icmp4/6()
>       when available, instead trusting the contents of flow->tap_omac.
> v12: - Using MAC_IS_UNDEF() instead of MAC_IS_ZERO()
>      - Comment update after feedback from Stefano
> v13: - No changes
> ---
>  icmp.c |  8 ++++++--
>  ndp.c  |  2 +-
>  tap.c  | 10 ++++++----
>  tap.h  |  4 ++--
>  udp.c  | 12 ++++++++++--
>  5 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 6dffafb..93b394a 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -125,17 +125,21 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
>  	flow_dbg(pingf, "echo reply to tap, ID: %"PRIu16", seq: %"PRIu16,
>  		 ini->eport, seq);
>  
> +	/* Find if neighbour table has a recorded MAC address */

Nit: "check if", or "find out if", or "find MAC address ...", but "find
if" is not really a common way to express this.

The rest of the series looks good to me.

-- 
Stefano