On 14/08/09 12:58 AM, Eric Cheng wrote:
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.
No. The entire point of the flag is to avoid any duplication,
be it dupmsg or copymsg. They're simply not required with BPF
and do nothing except making things run slower. When using BPF
to capture packets, there should be no need to allocate any
new data structures in the process of capturing the packet
and the only time packet data is copied is when it matches the
filter being applied to the network traffic.
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.
Accept.
So with the above change to use "new_open", it does become
worthwhile to have pfp_close().
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.
Hmmm!
What would be nice is a way to tell mac_header_info() to optionally
iterate through layer 2 headers until the last one is found and
extract the "real" SAP into mhi_bindsap. Looking at
dls_link_header_info(), I can recall looking at it. In its present
form, it isn't useful because it requires a dls_link_t *. But
changing it to use mac_handle_t is quite easy (see attached) and
that would make it possible to use that function rather than writing
another version of it. But changing the first parameter of
dls_link_header_info() implies that the function name and location
should also change...which is getting outside of the scope of what
this project is about. The functional requirement is to provide the
raw ethernet packet.
So I think what I'll do is:
1) change the code to mimic dls_link_header_info()
2) leave the code as is (without the MAC_PROMISC_FLAGS_VLAN_TAG_STRIP
flag)
3) file a bug that mentions dls_link_header_info() can work with
a mac_handle_t instead of a dls_link_t * and when the mac/dls
cleanup gets into gear, it can take care of both pieces of
code then. See CR#6872007
Darren
diff -r 4591dcf8c6ea usr/src/uts/common/io/dld/dld_str.c
--- a/usr/src/uts/common/io/dld/dld_str.c Thu Aug 13 22:57:44 2009 -0700
+++ b/usr/src/uts/common/io/dld/dld_str.c Fri Aug 14 10:25:31 2009 -0700
@@ -885,7 +885,7 @@
size += MBLKL(bp);
}
- if (dls_link_header_info(dsp->ds_dlp, mp, &mhi) != 0)
+ if (dls_link_header_info(dsp->ds_dlp->dl_mh, mp, &mhi) != 0)
goto discard;
mac_sdu_get(dsp->ds_mh, NULL, &max_sdu);
@@ -1400,7 +1400,7 @@
/*
* Get the packet header information.
*/
- if (dls_link_header_info(dsp->ds_dlp, mp, &mhi) != 0)
+ if (dls_link_header_info(dsp->ds_dlp->dl_mh, mp, &mhi) != 0)
return (NULL);
/*
diff -r 4591dcf8c6ea usr/src/uts/common/io/dls/dls_link.c
--- a/usr/src/uts/common/io/dls/dls_link.c Thu Aug 13 22:57:44 2009 -0700
+++ b/usr/src/uts/common/io/dls/dls_link.c Fri Aug 14 10:25:31 2009 -0700
@@ -110,7 +110,7 @@
*/
#define DLS_PREPARE_PKT(dlp, mp, mhip, err) {
\
mblk_t *nextp = (mp)->b_next; \
- if (((err) = dls_link_header_info((dlp), (mp), (mhip))) == 0) { \
+ if (((err) = dls_link_header_info((dlp)->dl_mh, (mp), (mhip))) == 0) {\
DLS_STRIP_PADDING((mhip)->mhi_pktsize, (mp)); \
if (MBLKL((mp)) < (mhip)->mhi_hdrsize) { \
mblk_t *newmp; \
@@ -120,7 +120,7 @@
(mp)->b_next = NULL; \
freemsg((mp)); \
(mp) = newmp; \
- VERIFY(dls_link_header_info((dlp), \
+ VERIFY(dls_link_header_info((dlp)->dl_mh,\
(mp), (mhip)) == 0); \
(mp)->b_next = nextp; \
(mp)->b_rptr += (mhip)->mhi_hdrsize; \
@@ -1052,9 +1052,9 @@
}
int
-dls_link_header_info(dls_link_t *dlp, mblk_t *mp, mac_header_info_t *mhip)
+dls_link_header_info(mac_handle_t mh, mblk_t *mp, mac_header_info_t *mhip)
{
- boolean_t is_ethernet = (dlp->dl_mip->mi_media == DL_ETHER);
+ boolean_t is_ethernet = (mac_type(mh) == DL_ETHER);
int err = 0;
/*
@@ -1062,7 +1062,7 @@
*/
ASSERT(IS_P2ALIGNED(mp->b_rptr, sizeof (uint16_t)));
- if ((err = mac_header_info(dlp->dl_mh, mp, mhip)) != 0)
+ if ((err = mac_header_info(mh, mp, mhip)) != 0)
return (err);
/*
@@ -1090,7 +1090,7 @@
}
evhp = (struct ether_vlan_header *)mp->b_rptr;
sap = ntohs(evhp->ether_type);
- (void) mac_sap_verify(dlp->dl_mh, sap, &mhip->mhi_bindsap);
+ (void) mac_sap_verify(mh, sap, &mhip->mhi_bindsap);
mhip->mhi_hdrsize = sizeof (struct ether_vlan_header);
mhip->mhi_tci = ntohs(evhp->ether_tci);
mhip->mhi_istagged = B_TRUE;
diff -r 4591dcf8c6ea usr/src/uts/common/sys/dls_impl.h
--- a/usr/src/uts/common/sys/dls_impl.h Thu Aug 13 22:57:44 2009 -0700
+++ b/usr/src/uts/common/sys/dls_impl.h Fri Aug 14 10:25:31 2009 -0700
@@ -84,7 +84,7 @@
extern int dls_link_rele_by_name(const char *);
extern void dls_link_add(dls_link_t *, uint32_t, dld_str_t *);
extern void dls_link_remove(dls_link_t *, dld_str_t *);
-extern int dls_link_header_info(dls_link_t *, mblk_t *,
+extern int dls_link_header_info(mac_handle_t, mblk_t *,
mac_header_info_t *);
extern int dls_link_getzid(const char *, zoneid_t *);
extern int dls_link_setzid(const char *, zoneid_t);
_______________________________________________
networking-discuss mailing list
[email protected]
