From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RxaGW-0002vO-QY for qemu-devel@nongnu.org; Wed, 15 Feb 2012 03:31:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RxaGS-0003JC-Fu for qemu-devel@nongnu.org; Wed, 15 Feb 2012 03:31:04 -0500 Received: from fmmailgate04.web.de ([217.72.192.242]:33735) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RxaGS-0003J8-6D for qemu-devel@nongnu.org; Wed, 15 Feb 2012 03:31:00 -0500 Received: from moweb002.kundenserver.de (moweb002.kundenserver.de [172.19.20.108]) by fmmailgate04.web.de (Postfix) with ESMTP id A312371B5220 for ; Wed, 15 Feb 2012 09:30:50 +0100 (CET) Message-ID: <4F3B6D36.1030808@web.de> Date: Wed, 15 Feb 2012 09:30:46 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1329293587-16246-1-git-send-email-zwu.kernel@gmail.com> In-Reply-To: <1329293587-16246-1-git-send-email-zwu.kernel@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig2ECDDBDF726CC46B4CE0888D" Subject: Re: [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zwu.kernel@gmail.com Cc: aliguori@us.ibm.com, Zhi Yong Wu , qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, mst@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig2ECDDBDF726CC46B4CE0888D Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable On 2012-02-15 09:13, zwu.kernel@gmail.com wrote: > From: Zhi Yong Wu >=20 > This patch fixes the slirp crash in current QEMU upstream. >=20 > Signed-off-by: Zhi Yong Wu > --- > slirp/if.c | 37 ++++++++++++++++++++++++++++++------- > slirp/mbuf.c | 3 +-- > 2 files changed, 31 insertions(+), 9 deletions(-) >=20 > diff --git a/slirp/if.c b/slirp/if.c > index 8e0cac2..f7f8577 100644 > --- a/slirp/if.c > +++ b/slirp/if.c > @@ -20,8 +20,15 @@ ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead) > static void > ifs_remque(struct mbuf *ifm) > { > - ifm->ifs_prev->ifs_next =3D ifm->ifs_next; > - ifm->ifs_next->ifs_prev =3D ifm->ifs_prev; > + if (ifm->ifs_next->ifs_next =3D=3D ifm > + && ifm->ifs_next->ifs_prev =3D=3D ifm) { > + ifs_init(ifm->ifs_next); > + } else { > + ifm->ifs_prev->ifs_next =3D ifm->ifs_next; > + ifm->ifs_next->ifs_prev =3D ifm->ifs_prev; > + } > + > + ifs_init(ifm); This looks, well, interesting. Can you explain the logic? We really need to document the queuing mechanics. > } > =20 > void > @@ -154,14 +161,18 @@ if_start(Slirp *slirp) > { > uint64_t now =3D qemu_get_clock_ns(rt_clock); > int requeued =3D 0; > - struct mbuf *ifm, *ifqt; > + struct mbuf *ifm, *ifqt, *ifm_next; > =20 > - DEBUG_CALL("if_start"); > + DEBUG_CALL("if_start"); > =20 > - if (slirp->if_queued =3D=3D 0) > - return; /* Nothing to do */ > + if (slirp->if_queued =3D=3D 0) > + return; /* Nothing to do */ Even if the result looks funny, let's not touch lines just for indention (and braces would be missing anyway). > + > + slirp->next_m =3D &slirp->if_batchq; Have you understood the difference between the natural order of if_batchq and next_m? I still wonder what the latter is good for. > =20 > again: > + ifm_next =3D NULL; > + > /* check if we can really output */ > if (!slirp_can_output(slirp->opaque)) > return; > @@ -190,6 +201,7 @@ if_start(Slirp *slirp) > /* If there are more packets for this session, re-queue them */ > if (ifm->ifs_next !=3D /* ifm->ifs_prev !=3D */ ifm) { > insque(ifm->ifs_next, ifqt); > + ifm_next =3D ifm->ifs_next; > ifs_remque(ifm); > } > =20 > @@ -209,7 +221,18 @@ if_start(Slirp *slirp) > m_free(ifm); > } else { > /* re-queue */ > - insque(ifm, ifqt); > + if (ifm_next) { > + /*restore the original state of bachq*/ > + remque(ifm_next); > + insque(ifm, ifqt); > + ifm_next->ifs_prev->ifs_next =3D ifm; > + ifm->ifs_prev =3D ifm_next->ifs_prev; > + ifm->ifs_next =3D ifm_next; > + ifm_next->ifs_prev =3D ifm; So is this only about the correct ordering or also about pointer correctness? > + } else { > + insque(ifm, ifqt); > + } > + > requeued++; > } > } > diff --git a/slirp/mbuf.c b/slirp/mbuf.c > index c699c75..f429c0a 100644 > --- a/slirp/mbuf.c > +++ b/slirp/mbuf.c > @@ -68,8 +68,7 @@ m_get(Slirp *slirp) > m->m_size =3D SLIRP_MSIZE - offsetof(struct mbuf, m_dat); > m->m_data =3D m->m_dat; > m->m_len =3D 0; > - m->m_nextpkt =3D NULL; > - m->m_prevpkt =3D NULL; > + ifs_init(m); > m->arp_requested =3D false; > m->expiration_date =3D (uint64_t)-1; > end_error: Thanks for digging into this, but I really think it needs more comments and potentially even some cleanups. Jan --------------enig2ECDDBDF726CC46B4CE0888D Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk87bTYACgkQitSsb3rl5xSU8QCgw4k9XDAib8JtdMIHyZ3mbQUL iB0An0a7e61KgVft4VE3+WPWxCPwUQ0W =ZyUG -----END PGP SIGNATURE----- --------------enig2ECDDBDF726CC46B4CE0888D--