On Thu, Sep 23, 2010 at 11:12:48AM +0200, Mike Belopuhov wrote: > On Thu, Sep 23, 2010 at 09:57 +0200, Bret S. Lambert wrote: > > No objection in principle (consolidation of code in mbufs is > > one of my currently-stalled-by-dayjob projects); comment inline. > > > > > > > > OK? > > > > > > Index: netinet/ip_esp.c > > > =================================================================== > > > RCS file: /home/cvs/src/sys/netinet/ip_esp.c,v > > > retrieving revision 1.112 > > > diff -u -p -r1.112 ip_esp.c > > > --- netinet/ip_esp.c 22 Sep 2010 13:40:05 -0000 1.112 > > > +++ netinet/ip_esp.c 22 Sep 2010 15:47:38 -0000 > > > @@ -932,9 +932,10 @@ esp_output(struct mbuf *m, struct tdb *t > > > * Add padding -- better to do it ourselves than use the crypto engine, > > > * although if/when we support compression, we'd have to do that. > > > */ > > > - pad = (u_char *) m_pad(m, padding + alen); > > > + mo = m_inject(m, m->m_pkthdr.len, padding + alen, M_DONTWAIT); > > > + pad = mtod(mo, u_char *); > > > > Sure, but this code creates a NULL pointer dereference; > > mtod() isn't a function that does anything smart, as > > is evidenced by the explanation in the mbuf man page: > > > > #define mtod(m,t) ((t)((m)->m_data)) > > > > /* Yeurk! */ > > new diff. thanks for looking through!
It looks sane enough to me, although you might also take a look at m_pulldown to ensure X contiguous bytes at offset Y in an mbuf chain, as, IIRC, that has the possibility of not requiring a new mbuf allocation if there truly is enough space in an existing mbuf. But, it's your bikeshed, you can choose whether it's grape- or strawberry-flavored. > > Index: netinet/ip_esp.c > =================================================================== > RCS file: /home/cvs/src/sys/netinet/ip_esp.c,v > retrieving revision 1.112 > diff -u -p -r1.112 ip_esp.c > --- netinet/ip_esp.c 22 Sep 2010 13:40:05 -0000 1.112 > +++ netinet/ip_esp.c 23 Sep 2010 09:05:40 -0000 > @@ -932,12 +932,13 @@ esp_output(struct mbuf *m, struct tdb *t > * Add padding -- better to do it ourselves than use the crypto engine, > * although if/when we support compression, we'd have to do that. > */ > - pad = (u_char *) m_pad(m, padding + alen); > - if (pad == NULL) { > - DPRINTF(("esp_output(): m_pad() failed for SA %s/%08x\n", > + mo = m_inject(m, m->m_pkthdr.len, padding + alen, M_DONTWAIT); > + if (mo == NULL) { > + DPRINTF(("esp_output(): m_inject failed for SA %s/%08x\n", > ipsp_address(tdb->tdb_dst), ntohl(tdb->tdb_spi))); > return ENOBUFS; > } > + pad = mtod(mo, u_char *); > > /* Self-describing or random padding ? */ > if (!(tdb->tdb_flags & TDBF_RANDOMPADDING)) > @@ -1178,77 +1179,4 @@ checkreplaywindow32(u_int32_t seq, u_int > > *bitmap |= (((u_int32_t) 1) << diff); > return 0; > -} > - > -/* > - * m_pad(m, n) pads <m> with <n> bytes at the end. The packet header > - * length is updated, and a pointer to the first byte of the padding > - * (which is guaranteed to be all in one mbuf) is returned. > - */ > - > -caddr_t > -m_pad(struct mbuf *m, int n) > -{ > - struct mbuf *m0, *m1; > - int len, pad; > - caddr_t retval; > - > - if (n <= 0) { /* No stupid arguments. */ > - DPRINTF(("m_pad(): pad length invalid (%d)\n", n)); > - m_freem(m); > - return NULL; > - } > - > - len = m->m_pkthdr.len; > - pad = n; > - m0 = m; > - > - while (m0->m_len < len) { > - len -= m0->m_len; > - m0 = m0->m_next; > - } > - > - if (m0->m_len != len) { > - DPRINTF(("m_pad(): length mismatch (should be %d instead of " > - "%d)\n", m->m_pkthdr.len, > - m->m_pkthdr.len + m0->m_len - len)); > - > - m_freem(m); > - return NULL; > - } > - > - /* Check for zero-length trailing mbufs, and find the last one. */ > - for (m1 = m0; m1->m_next; m1 = m1->m_next) { > - if (m1->m_next->m_len != 0) { > - DPRINTF(("m_pad(): length mismatch (should be %d " > - "instead of %d)\n", m->m_pkthdr.len, > - m->m_pkthdr.len + m1->m_next->m_len)); > - > - m_freem(m); > - return NULL; > - } > - > - m0 = m1->m_next; > - } > - > - if ((m0->m_flags & M_EXT) || > - m0->m_data + m0->m_len + pad >= &(m0->m_dat[MLEN])) { > - /* Add an mbuf to the chain. */ > - MGET(m1, M_DONTWAIT, MT_DATA); > - if (m1 == 0) { > - m_freem(m0); > - DPRINTF(("m_pad(): cannot append\n")); > - return NULL; > - } > - > - m0->m_next = m1; > - m0 = m1; > - m0->m_len = 0; > - } > - > - retval = m0->m_data + m0->m_len; > - m0->m_len += pad; > - m->m_pkthdr.len += pad; > - > - return retval; > } > Index: netinet/ip_ipsp.h > =================================================================== > RCS file: /home/cvs/src/sys/netinet/ip_ipsp.h,v > retrieving revision 1.144 > diff -u -p -r1.144 ip_ipsp.h > --- netinet/ip_ipsp.h 9 Jul 2010 16:58:06 -0000 1.144 > +++ netinet/ip_ipsp.h 22 Sep 2010 15:49:32 -0000 > @@ -624,9 +624,6 @@ extern int tcp_signature_tdb_input(struc > extern int tcp_signature_tdb_output(struct mbuf *, struct tdb *, > struct mbuf **, int, int); > > -/* Padding */ > -extern caddr_t m_pad(struct mbuf *, int); > - > /* Replay window */ > extern int checkreplaywindow32(u_int32_t, u_int32_t, u_int32_t *, u_int32_t, > u_int32_t *, int);