Seb,
Still working through the main iptun source files, but here's my next
round of feedback.
Webrev:
http://cr.opensolaris.org/~seb/clearview-iptun-cr3/
uts/common/inet/iptun.h:
* Seems odd to have four different prefixes in this header
(IPTUN_, ITK_, tun_, and IPV6_). The latter two define objects
that appear more suited to an IPv6 header.
* 58-59: Seems odd these aren't (5) and (6).
* 69: This constraint may pose some future challenges if we need
to e.g. add members to the middle of this structure and then
support an old userland (e.g., Solaris 11 zones on a Solaris 12
box). (That said, I don't think it's worth worrying about it --
and don't get me started on the nightmare associated with
mix-and-match userlands on a common kernel.)
uts/common/inet/iptun/iptundummy.c:
* 38: Seem strange to have a local header in the middle of the
system header list; is there a technical need for this?
* 45: STREAMS module names can only be 8 (FMNAMESZ) characters;
see 4923054 for an example of the consequences of not doing
this. Suggest renaming to "iptunq" or somesuch.
* 67-70: Comment seems redundant given 27-32.
* 83-84: Given that this is not installed as a module, this seems
needless.
* 99-100: Not due to your wad, but it seems like this should be
done by ipcl_conn_create().
* 129: Why is ip_quiesce_conn() done after qprocsoff()?
* 130: Seems needless since ipcl_conn_destroy() will ASSERT() that
conn_ref == 0 and this function doesn't really care about the
conn.
uts/common/inet/iptun/iptundummyddi.c:
* 26-32: Not sure we need the same comment as iptundummy.c here.
uts/common/inet/iptun/iptun_impl.h:
* 54: Unclear why IPTUN_HASH_KEY is here rather than iptun.c.
* 175: Would be good to explain the protection for all the fields.
uts/common/inet/iptun/iptun_dev.c:
* 53-83: Seems like DDI_DEFINE_STREAM_OPS() could just be used.
* 106: Needless cast.
* 233-234: Is this just paranoia? If we're really dependent
on iptun_count(), then what prevents iptun_tunnelcount changing
immediately after the check, given that we're not holding any
locks?
* 219-220, 240-241: Pseudo-devices will never be called with
DDI_RESUME or DDI_SUSPEND.
* 254: On a non-DEBUG system, a kmem cache object will not
generally go back through the destructor/constructor path
when it's freed. As such, seems like iptun_alloc() may have
some other initialization to do to ensure the iptun_t does
not have any residual state from a previous incarnation
(e.g., stale values like IPTUN_HOPLIMIT set in iptun_flags).
uts/common/inet/iptun/iptun_ctl.c:
* 27-31: Comment seems only tangentially related to the source.
* 34: Probably no need for sys/priv_names.h here.
--
meem
