Re: Do not use rn_refines in if.c

[Alexander Bluhm]

On Mon, Oct 10, 2016 at 08:15:19PM +0200, Claudio Jeker wrote:
> So rn_refines() is a function from the deep underbelly of radix code.
> In my opinion if.c should not use this function especially now that we use
> art for routing table lookups. Instead I implemented a simple
> ifa_netmask_cmp() function that does what the code expects and compares
> the two netmasks. It is a bit more complictated than a simple memcmp()
> because netmasks can be encoded in a few ways (more or less padding
> bytes). Also the code assumes consecutive netmasks since that is the only
> thing we allow on interfaces and IP routes anyway.

I think this is not the same result as before, even for consecutive
netmasks.

You basically implemented a ifa_netmask_equal() which checks wether
both masks are equal.  But according to the comment ifa_ifwithnet()
should find the most specific netmask.

Could you implement a ifa_netmask_cmp() that has the negative, zero,
positive return values like memcmp()?

> +int
> +ifa_netmask_cmp(struct sockaddr *sa1, struct sockaddr *sa2)
> +{
> + int len, rv, diff, i;
> + char*cp;
> +
> + len = sa1->sa_len > sa2->sa_len ? sa2->sa_len : sa1->sa_len;

I think min(sa1->sa_len, sa2->sa_len) is easier to read.

> + len -= offsetof(struct sockaddr, sa_data);
> +
> + rv = memcmp(sa1->sa_data, sa2->sa_data, len);
> + if (rv != 0)
> + return (0);

Could we return rv here?

> + /* if sa_len is different make sure the longer is only 0 padded */
> + diff = sa1->sa_len - sa2->sa_len;
> + if (diff > 0) {
> + cp = sa1->sa_data;
> + for (i = 0; i < diff; i++) {
> + if (cp[len + i] != 0)
> + return (0);

And 1 here...

> + }
> + } else if (diff < 0) {
> + cp = sa2->sa_data;
> + for (i = 0; i < -diff; i++) {
> + if (cp[len + i] != 0)
> + return (0);

And -1 here...

> + }
> + }
> + return (1);

And 0 here.

As we assume that the netmasks are continous, it should be sufficent
to check cp[len] != 0 instead off looping over diff.

> +}
> +
>  /*
>   * Find an interface on a specific network.  If many, choice
>   * is most specific found.
> @@ -1311,9 +1342,8 @@ ifa_ifwithnet(struct sockaddr *sa, u_int
>   if ((*cp++ ^ *cp2++) & *cp3++)
>   /* want to continue for() loop */
>   goto next;
> - if (ifa_maybe == 0 ||
> - rn_refines((caddr_t)ifa->ifa_netmask,
> - (caddr_t)ifa_maybe->ifa_netmask))
> + if (ifa_maybe == 0 || ifa_netmask_cmp(ifa->ifa_netmask,
> + ifa_maybe->ifa_netmask))

I we check ifa_netmask_cmp(ifa->ifa_netmask, ifa_maybe->ifa_netmask) > 0
here, we should find the most specific netmask.

>   ifa_maybe = ifa;
>   }
>   }

bluhm

Re: Do not use rn_refines in if.c

[Mike Belopuhov]

On 10 October 2016 at 20:15, Claudio Jeker  wrote:
> So rn_refines() is a function from the deep underbelly of radix code.
> In my opinion if.c should not use this function especially now that we use
> art for routing table lookups. Instead I implemented a simple
> ifa_netmask_cmp() function that does what the code expects and compares
> the two netmasks. It is a bit more complictated than a simple memcmp()
> because netmasks can be encoded in a few ways (more or less padding
> bytes). Also the code assumes consecutive netmasks since that is the only
> thing we allow on interfaces and IP routes anyway.
>
> OK?

I think this is great.  I can't even tell if this does the same as rn_refines
cause it's nuts :-)  But if it does, then I'm all for your version (-: