On Mon, Jul 27, 2009 at 07:01:58PM -0700, Darren Reed wrote:
> http://cr.opensolaris.org/~darrenr/onnv-pcapture/
>
Hi darren,
Here are my comments for the mac and sockmod_pfp components:
dls_link.c:
930:
dls_link_getzid() seems to be unused.
mac_client.c:
3253-3254:
I think you can't assume that the mp_copy is still valid after
invoking mpip->mpi_fn(). mp_copy could already be freed.
to fix this, you'll probably need to move the mpi_no_copy logic
to mac_promisc_dispatch() and mac_promisc_client_dispatch(). the
b_next should be saved before calling mac_promisc_dispatch_one().
also, if mpi_no_copy is set, the mp_chain has to be freed
right after mac_promisc_dispatch()/mac_promisc_client_dispatch()
because you can't let the same mp_chain come up two different paths.
bpf.c:
1710-1720:
if I read this correctly, this is passing the mac name to
mac_client_open() which will result in the mac name being copied
to bif_ifname. I think you want the real link name right?
to get the real link name, the name arg to mac_client_open()
should be NULL and flags should include
MAC_OPEN_FLAGS_USE_DATALINK_NAME.
also, in several places in this file you have something like:
if (bpf_debug)
cmn_err(CE_WARN, "...");
it may be better to put this into a macro.
bpf_mac.c:
102:
do you need to return the correct size for non-ether macs?
the bpr_hdr_length entry point also seems to be unused.
sockmod_pfp.c:
1211:
you should consider adding a pfp_close() function symmetrical
to pfp_open_index() for closing mh/mch.
1223:
similar problem as bpf.c.
you should pass a NULL name and MAC_OPEN_FLAGS_USE_DATALINK_NAME
flag to mac_client_open(), then use mac_client_name() to get
the link name and pass it to zone_access_datalink().
716,719:
if you did the pfp_open_index() at 660, these conditions must
be true. you could set a boolean at 660 and use it instead
of checking mh/mch. something like:
if (new_open) {
ASSERT(mch != ps->ps_mch);
ASSERT(mh != ps->ps_mh);
pfp_close(mch, mh);
}
722:
mp could be uninitialized if you jumped from 674 and this would
free invalid memory.
330:
you need to specify MAC_PROMISC_FLAGS_VLAN_TAG_STRIP in
mac_promisc_add() otherwise the mhi_bindsap here will be
incorrect for vlan packets.
356:
shouldn't you use msgdsize() instead of MBLKL?
408:
mhi_saddr could possibly be NULL (for IB links)
976:
why doesn't this function use dls_mgmt_get_linkid()?
955,982:
should this be copyin() instead of bcopy()?
1150:
if possible, try to limit bf_len/bf_insns to some reasonable
number so you don't allocate too much kernel memory.
1180:
need to return an error if bpf_validate() fails.
1160:
maybe pfp_release_bpf() should be done after bpf_validate().
eric
