On (07/29/09 16:14), Sebastien Roy wrote: Files reviewed with no comments usr/src/lib/libdladm/Makefile usr/src/lib/libdladm/Makefile.com usr/src/cmd/cmd-inet/usr.sbin/ifconfig/defs.h usr/src/lib/libdladm/common/libdladm.[ch] usr/src/lib/libdladm/common/libdladm_impl.h usr/src/lib/libdladm/common/libdlaggr.c usr/src/lib/libdladm/common/libdllink.[ch] usr/src/lib/libdladm/common/libdlmgmt.[ch] usr/src/lib/libdladm/common/libdlvnic.c usr/src/lib/libinetcfg/common/inetcfg.[ch] usr/src/lib/libinetcfg/common/mapfile-vers usr/src/lib/libinetcfg/common/usage.c usr/src/lib/libinetutil/common/libinetutil.h usr/src/uts/common/inet/Makefile
I did not review the libdlpi files- hopefully someone else is looking at these- please let me know if I should look at these as well. I also glanced through the inet/ip and iptun files- relevant comments below, but I can give you a list of the files reviewed, if this is useful/needed. usr/src/cmd/cmd-inet/usr.sbin/Makefile - ndd can also be added to DLADM_PROG. And then we can remove line 154 from the file. usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c - "dladm_link_open()" is a confusing name for the function (sounds like something from dladm_open()". Suggest ifconfig_dl_open() or something similar to show that it is local to ifconfig. - Instead of doing pairs of dladm_open/dladm_close, suggest having a global dladm_handle that is opened once in main and closed on exit. dladm_link_open() can use that handle, and it can be reused for "ifconfig -a <..>" commands like "ifconfig -a ether" and "ifconfig -a plumb". - string passed to str_exit() needs to be gettext'ed? - missing \n after line 4185 - Question at line 3915: would you be able to open a datalink if it has been assigned to a NGZ (in the excl zones case)? usr/src/cmd/dladm/dladm.c - line 360: turns out the corrrect spelling is "parsable", but we've documented ourselves into having to support both. - missing \n after line 986, 3666, 3685 - line 991, 992: shouldn't the field width really be INET6_ADDRSTRLEN? usr/src/cmd/dladm/dladm.xcl - missing up-iptun, down-iptun, tunnel-type, tunnel-src, tunnel-dst, ipv4, ipv6, DESTINATION and several other strings. usr/src/lib/libdladm/common/linkprop.c - line 2063: good catch :-) usr/src/lib/libinetutil/common/ifspec.c - line 116 not your change, but shouldn't this be more efficently done as iflen = strnlen(ifnam, LIFNAMSIZ); usr/src/lib/libldadm/common/libdliptun.c - lines 564-566: does it really matter? For all we know, the "old_params" may not be valid any more either, if, for example, the iptun_param_src has been marked down. So how terrible is it to just leave the db with the new params? - Question: line 605: why is the explicit dladm_init_linkprop needed (as opposed to just having the dls layer suck down the properties)? usr/src//uts/common/inet/iptun/iptun.c - line 1535: suggest bzero(itk, sizeof (*itk)) to future-proof against changest to the type of itk. usr/src/uts/common/inet/ip/ip_ndp.c - do you need symmetric changes to ndp_lookup_then_add_v4 that also add the new ipv4 nce with hwaddr == ill_dest_addr for tunnel interfaces? usr/src/uts/common/inet/ip/ipclassifier.c - line 311: 389 is an odd number (no pun intended!) - how did you come up with that default? Adding some comments about how this default was chose would be useful. (Esp since this choice also impacts whether or not % needs to be used in IPCL_IPTUN_HASH) usr/src/uts/common/inet/iptun/iptun.c - why not just do a msgpullup at line 2043. - General question about how all this works.. it's possible that I'm missing something obvious here, so I'm looking for clarification- if I set up an interface to be an ipv4 tunnel interface using dladm, then I would have guessed that, when I send an ip packet "mp" down to that tunnel driver, then iptun_ouput() would slap on the appropriate IP "link" headers for the tunnel and send the packet down, using some possibly cached route (i.e., ire) info to find the nexthop/interface to use. this should be done regardless of whether mp is IPv4 or IPv6. So I was thrown off by the way in which (e.g.,) iptun_out_process_ipv4 grovels in the packet to determine stuff. e.g., recursive-ness in tunnels would be easier to detect and forbid when the tunnel is constructed and marked RUNNING, right? And if we consider that a tunnel tun1 with outer header (A -> B) and inner header (C -> B) is recursive [#] then what about a tunnel which has outer/inner header (A -> B) / (C -> D) where the route for B points at a tunnel tun2 that sets up (C1 -> D) as the outer header? Evenutually the recursion will be caught by one of the tunnels spilling over the encaps limit, so the test in iptun_out_process* is actually needless, right? [#] I'm actually not convinced this is a good test for recursion- why can't I set up 2 paths for B, one of which goes through tun1, and the other through my default gw on ethernet? The constraint seems to be that I *must* have outer_dst != inner_dst, and I'm not seeing why this is needed- all that the tunnel needs to do is to make sure that, after it encapsulates the packet, the route selected for the encapsulated packet does not point through itself? (and even more confusing: we drop the packet on the input path when inner_dst == outer_dst even if both of these are locally hosted addresses? I would expect a host that receives such a packet to accept it, assuming everything else checks out).
