On Wed, Feb 15, 2012 at 4:30 PM, Jan Kiszka <jan.kis...@web.de> wrote:
> On 2012-02-15 09:13, zwu.ker...@gmail.com wrote:
>> From: Zhi Yong Wu <wu...@linux.vnet.ibm.com>
>>
>> This patch fixes the slirp crash in current QEMU upstream.
>>
>> Signed-off-by: Zhi Yong Wu <wu...@linux.vnet.ibm.com>
>> ---
>> slirp/if.c | 37 ++++++++++++++++++++++++++++++-------
>> slirp/mbuf.c | 3 +--
>> 2 files changed, 31 insertions(+), 9 deletions(-)
>>
>> 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 = ifm->ifs_next;
>> - ifm->ifs_next->ifs_prev = ifm->ifs_prev;
>> + if (ifm->ifs_next->ifs_next == ifm
>> + && ifm->ifs_next->ifs_prev == ifm) {
>> + ifs_init(ifm->ifs_next);
>> + } else {
>> + ifm->ifs_prev->ifs_next = ifm->ifs_next;
>> + ifm->ifs_next->ifs_prev = ifm->ifs_prev;
>> + }
>> +
>> + ifs_init(ifm);
>
> This looks, well, interesting. Can you explain the logic? We really need
> to document the queuing mechanics.
ifm->ifs_prev->ifs_next = ifm->ifs_next;
ifm->ifs_next->ifs_prev = ifm->ifs_prev;
+ ifs_init(ifm);
Sorry, actually we only need to add one line of code.
>
>> }
>>
>> void
>> @@ -154,14 +161,18 @@ if_start(Slirp *slirp)
>> {
>> uint64_t now = qemu_get_clock_ns(rt_clock);
>> int requeued = 0;
>> - struct mbuf *ifm, *ifqt;
>> + struct mbuf *ifm, *ifqt, *ifm_next;
>>
>> - DEBUG_CALL("if_start");
>> + DEBUG_CALL("if_start");
>>
>> - if (slirp->if_queued == 0)
>> - return; /* Nothing to do */
>> + if (slirp->if_queued == 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).
OK. undo them to their original state.
>
>> +
>> + slirp->next_m = &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.
Sorry, the line of code should be removed. next_m will point to next
packet which will be handled, if there's multiple session, it will
point to the head packet for next session; if there's one session, it
will point to batchq.
>
>>
>> again:
>> + ifm_next = 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 != /* ifm->ifs_prev != */ ifm) {
>> insque(ifm->ifs_next, ifqt);
>> + ifm_next = ifm->ifs_next;
>> ifs_remque(ifm);
>> }
>>
>> @@ -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 = ifm;
>> + ifm->ifs_prev = ifm_next->ifs_prev;
>> + ifm->ifs_next = ifm_next;
>> + ifm_next->ifs_prev = ifm;
>
> So is this only about the correct ordering or also about pointer
> correctness?
The former. If ifm need to be re-queued, it will be put to where it
originally is.
>
>> + } 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 = SLIRP_MSIZE - offsetof(struct mbuf, m_dat);
>> m->m_data = m->m_dat;
>> m->m_len = 0;
>> - m->m_nextpkt = NULL;
>> - m->m_prevpkt = NULL;
>> + ifs_init(m);
>> m->arp_requested = false;
>> m->expiration_date = (uint64_t)-1;
>> end_error:
>
> Thanks for digging into this, but I really think it needs more comments
> and potentially even some cleanups.
>
> Jan
>
--
Regards,
Zhi Yong Wu
