[crossbow-discuss] Code review for IP Instances

[Erik Nordmark]

sowmini.varadhan at Sun.COM wrote:
>  
> Erik, Yukun, Dong-Hai,
> 
> I haven't reviewed all the files, but focussed on the surya-related pieces. 
> Here's what I came up with:

Thanks for your review.

> files reviewed: ip_ndp.[ch], ip_ire.[ch], ip_ftable.[ch], netstack.c,
>                 ip_if.c, ip.c
> 
> - ip_ndp.h: need to remove the foollowing two decls (Comments have to
>   be adjusted as well):
>       extern ndp_g_t  ndp4, ndp6;

Will do.

> - ip_ftable.c: ipfil_sendpkt() lines 1261-1266;
>   Is it possible to call ipfil_sendpkt from a non-global, exclusive zone
>   which is itself partitioned into non-exclusive zones, i.e.,
>   something like  ipfil_sendpkt(... , zoneid == 5) from within
>   zone#3?

Philosophical question since ipfil_sendpkt() is called from the kernel, 
and the kernel doesn't operate with an implicit zoneid.

If you are asking whether somebody can write kernel code that "jumps" 
between IP Instances, that is certainly possible.

One can do that not only with this function, but also in general by 
using netstack_find_by_stackid() and then using the netstack_t pointer 
with other functions.

> - ip_ftable.h line 64 and ip_ire.c line 2877: please clarify the
>   "no netstack_hold()" comment?  seems like the ipst is obtained from
>    the ill_ipst or conn_netstack->netstack_ip, or by marching down some
>   global netstack list, so what does the comment refer to?

It refers to the fact that we only do netstack_hold in particular cases, 
  such as the references from open streams (ill_t and conn_t's 
pointers). Internally within IP we rely on IP's ability to cleanup e.g. 
ire_t's when an ill goes away.

The design document (on opensolaris,org) has some added text to try to 
explain this. Is there something better we can do in the source code?

> - os/netstack.c: Isn't line 157 always true (the NRF_REGISTERED bit
>   just got set at line 149)? Similarly line 199 vs line 190;

Good point. Will fix.

> - ip_if.c: (nit) indentation is off in ill_ring_add() (e.g., line 2715-2721?)
> - ip_if.c: (nit) extra blank line at 4196

Will fix (even though the first part is code not added by us.)

> - ip_if.c phyint_assign_ifindex(): when the ips_ill_index wraps around,
>   we would skip the interface index of 1?  I think line 4765 should be
>       if (ipst->ips_ill_index == 0 && !ipst->ips_ill_index_wrap)
>                 ipst->ips_ill_index = 1;

That was not for the wraparound but for the initialization. But I'll 
move the initialization to ip_stack_init().

    Erik