dst_ill is different from ire->ire_ipif->ipif_ill only in the case of
IPMP and VNI.
So with the changes you are making, it should be fine, i guess.
thirumalai
Peter Memishian wrote:
>Thiru/Sowmini,
>
>There are a number of places in Nevada where we do something like:
>
> if ((dst_ill->ill_group != NULL) ||
>--> (ire->ire_ipif->ipif_flags & IPIF_DEPRECATED) ||
> (connp != NULL && ire->ire_zoneid != zoneid &&
> ire->ire_zoneid != ALL_ZONES) ||
> (dst_ill->ill_usesrc_ifindex != 0)) {
> ipaddr_t saddr = ire->ire_ipif->ipif_src_addr;
>--> src_ipif = ipif_select_source(dst_ill, saddr,
> zoneid); ^^^^^^^
>
>
>In particular, I thought the rule was that the ill associated with
>ire->ire_ipif was used for source address selection. Indeed, you can see
>on the first arrowed line that if the ire_ipif itself is not deprecated,
>we often just use it directly. However, as per the second arrowed line,
>in the case where we cannot (or should not, in the case of inbound
>load-spreading with IPMP) just use ire_ipif, we pass *dst_ill* rather
>than ire->ire_ipif->ipif_ill.
>
>This seems wrong. However, it doesn't cause any problems for Nevada in
>the IPMP case because ipif_select_source() will check if dst_ill is in a
>group and if so it will scan across all of the interfaces in the group.
>However, in my new design, there is a profound difference between the two:
>ire->ire_ipif->ipif_ill will refer to an IPMP interface (and thus will
>return a random available data address on that IPMP interface), whereas
>dst_ill will refer to an underlying interface (and thus will return a test
>address).
>
>Is there any issue with changing these callers to use
>ire->ire_ipif->ipif_ill instead? (Basic testing seems to suggest not.)
>
>As an aside, it seems like making this change would simplify VNI --
>currently, since ipif_select_source() passes in dst_ill rather than
>ire->ire_ipif->ipif_ill, ipif_select_source() needs to re-derive the VNI
>interface:
>
> if (ill->ill_usesrc_ifindex != 0) {
> usill = ill_lookup_on_ifindex(ill->ill_usesrc_ifindex,
> B_FALSE, NULL, NULL, NULL, NULL, ipst);
> if (usill != NULL)
> ill = usill; /* Select source from usesrc ILL */
> else
> return (NULL);
> }
>
>With the proposed change to the ipif_select_source() callers, I believe
>this code could just be removed.
>
>Please let me know your thoughts.
>
>
