On Mon, Jul 06, 2009 at 07:17:14PM -0700, Darren Reed wrote: > I'd like to invite people to review the PF_PACKET port of the > packet capture project. Apologies if some of the Makefile bits > seem like orphans, other changes are embedded with BPF work > that isn't yet ready for review. The primary file requiring review > here is sockmod_pfp.c, but please feel free to review others. > I'd like to receieve any comments by the end of next week > (17th of July 2009.) > > http://cr.opensolaris.org/~darrenr/pf_packet_review/
Hi Darren, The comments below are for sockmod_pfp.c. - There are a few code paths that use copyin/copyout explicitly, making them unsuitable for kernel sockets. Is it expected that PF_PACKET will only be used by userland sockets? There is unfortunately no mechanism for marking a module as "unavailable" for kernel consumers, which is maybe what is needed in this case. - There does not seem to be any flow control on the receive side. It is the responsibility of the socket module to assert flow control/drop packets in case the recv upcall returned ENOSPC. Sorry about the lack of documentation, but have a look at udp_ulp_recv() and udp_clr_flowctrl(). - line 93: Does pfp_kstats need to have global scope? Just declare it in sockpfp_init. - line 156: Using PF_PACKET rather than PFP would make for a better description. - line 210: empty line. - lines 217 - 218: Not needed, since kmem_cache_create cannot fail. - line 240: empty line - line 276: The error code should be 'EPROTOTYPE'. - lines 300 - 324: There is not much work in the ctor/dtor. Do you even need to use a kmem cache for creating pfpsocks? - line 389: Use MBLKL? - line 419: extra space. - lines 684, 689: Looks like you are missing some cleanup in these two error cases. Should you not close the mac and mac client? - lines 936, 962: The comments for these functions say that 'arg' could potentially be in user space, so I expect that it can handle kernel addresses as well. But the code always uses copyin. Should the comment or code be updated? - line 1005: optval points to a kernel buffer, so use bcopy. - line 1044: optval points to a kernel buffer. - lines 1139, 1146: optval points to kernel buffers. - line 1156: memory leak; you do not free fcode if the copyin fails. Cheers, Anders _______________________________________________ networking-discuss mailing list [email protected]
