On Thu, Aug 13, 2009 at 10:04:01PM -0700, Darren Reed wrote:
> >dls_link.c:
> >
> >930:
> >dls_link_getzid() seems to be unused.
> >
>
> Discuss.
> In testing, zone_access_datalink() didn't work as expected, thus it has
> been thrown to /dev/null and dls_link_getzid() used instead (code path
> now works as expected.)
>
I'm fine with keeping dls_link_getzid() if it's needed but do you know
if the zone_access_datalink() problem is related to the macname issue
I mentioned in sockmod_pfp:1223?
>
> >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.
> >
>
> Discuss.
> The basis of "mp_copy == mp" is the mpi_no_copy being true.
> Users of the interface that do not request the mblk to be copied
> (by using MAC_PROMISC_FLAGS_NO_COPY) are not entitled
> to modify the mblk_t that is passed in. That includes prohibition
> from free'ing them. Or is there some other code path that will
> free the mblk_t whilst mpi_fn() is being called?
>
no, it won't get freed.
I just think it's better to not change the expected behavior of
mpi_fn() because someone passed in a certain flag. I think it's
more consistent with other mac callbacks to let it consume the
packet.
If you're concerned about copying cost, you could do dupmsg() instead
of copymsg() if mpi_no_copy is set.
>
> >sockmod_pfp.c:
> >
> >1211:
> >you should consider adding a pfp_close() function symmetrical
> >to pfp_open_index() for closing mh/mch.
> >
>
> Reject.
> There would only be one use of pfp_close(), whilst there are multiple
> places that need pfp_open_index()
>
I don't mind if you keep it this way.
but from cscope, I see two uses of pfp_open_index(), in both these
cases I see mac_client_close()/mac_close() as well. so you should
have more than one use of pfp_close().
>
> >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);
> >}
> >
>
> Discuss.
> So you only wanted pfp_close() to do the mac_client_close and the
> mac_close? That seems hardly worth it... but the new_open change
> is worthwhile.
>
I found two cases but as I said I don't mind either way.
> >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.
> >
>
> Discuss.
> Does this comment apply to all uses of mac_promisc_add()?
> I couldn't find any that are at line 330...I think you mean 1330?
>
I was talking about this:
if ((ps->ps_proto != 0) && (ps->ps_proto != hdr.mhi_bindsap)) {
}
e.g.:
if someone binds to the ip sap, all inbound packets with a
vlan header will be dropped.
yes, this affects all calls to mac_promisc_add().
an alternative is to not strip the vlan tag, and skip to the real
sap yourself, like dls_link_header_info().
I'm not sure what's the correct semantics for pf_packet. it maybe
better to pass up a full raw header.
eric
