On Wed, Jan 12, 2022 at 04:21:13PM +0100, Christian Ehrhardt wrote:
> > > - if (len <= tail - head) {
> > > - /* there's enough space in the first mbuf */
> > > -
> > > - if (len > tail - mtod(m0, caddr_t)) {
> > > + if (!M_READONLY(m0) && len <= tail - head) {
> > > + /* we can copy everything into the first mbuf */
> > > + if (m0->m_len == 0) {
> > > + m0->m_data = head;
> >
> > Before your patch, m_data was effectively set to head - adj in this case,
> > because head += adj had not happened yet. Does this fix a bug in the old
> > code, or does it introduce a bug?
>
> Oh, you are right. If the first mbuf is empty but large enough
> to hold the pulled data, we previously did not honour the alignment.
> I admit that I didn't notice this semantic change but I still think
> that the new version of the code is what is supposed to happen.
I think the new code preserves the alignment and that is better.
> > > + } else if (len > tail - mtod(m0, caddr_t)) {
> > > /* need to memmove to make space at the end */
> > > memmove(head, mtod(m0, caddr_t), m0->m_len);
> > > m0->m_data = head;
> > > + len -= m0->m_len;
> > > }
> > > -
> > > - len -= m0->m_len;
I don't understand that chunk. Why do you move the "len -= m0->m_len" ?
In the case "m0->m_len != 0" and "len <= tail - mtod(m0, caddr_t)"
"m0->m_len" octets are already at the correct place.
Only "len - m0->m_len" octets have to be copied from the following mbufs.
I think it should be:
+ if (!M_READONLY(m0) && len <= tail - head) {
+ /* we can copy everything into the first mbuf */
+ if (m0->m_len == 0) {
+ m0->m_data = head;
+ } else if (len > tail - mtod(m0, caddr_t)) {
/* need to memmove to make space at the end */
memmove(head, mtod(m0, caddr_t), m0->m_len);
m0->m_data = head;
}
len -= m0->m_len;
} else {
bluhm
