Re: [PATCH v4 1/7] arp: Fix a handful of small warts

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Oct 2024 16:32:58 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This fixes a number of harmless but slightly ugly warts in the ARP
> resolution code:
>  * Use in4addr_any to represent 0.0.0.0 rather than hand constructing an
>    example.
>  * When comparing am->sip against 0.0.0.0 use sizeof(am->sip) instead of
>    sizeof(am->tip) (same value, but makes more logical sense)
>  * Described the guest's assigned address as such, rather than as "our
>    address" - that's not usually what we mean by "our address" these days
>  * Remove "we might have the same IP address" comment which I can't make
>    sense of in context (possibly it's relating to the statement below,
>    which already has its own comment?)

Right, yes, I added the part below later, in 64c0f20ab3f8 ("arp: Don't
resolve own, configured IPv4 address").

The first check comes from 2166c5872e9b ("arp: Don't answer announcements
from guest or namespace") instead. The commit title makes it obvious, the
comment not so much.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  arp.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index 53334da..d34b20e 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -59,14 +59,12 @@ int arp(const struct ctx *c, const struct pool *p)
>  	    ah->ar_op  != htons(ARPOP_REQUEST))
>  		return 1;
>  
> -	/* Discard announcements (but not 0.0.0.0 "probes"): we might have the
> -	 * same IP address, hide that.
> -	 */
> -	if (memcmp(am->sip, (unsigned char[4]){ 0 }, sizeof(am->tip)) &&
> +	/* Discard announcements, but not 0.0.0.0 "probes" */
> +	if (memcmp(am->sip, &in4addr_any, sizeof(am->sip)) &&
>  	    !memcmp(am->sip, am->tip, sizeof(am->sip)))
>  		return 1;
>  
> -	/* Don't resolve our own address, either. */
> +	/* Don't resolve guest's assigned address, either. */

Nit: "the guest's assigned address".

>  	if (!memcmp(am->tip, &c->ip4.addr, sizeof(am->tip)))
>  		return 1;

-- 
Stefano