[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qe
From: |
Yoshiaki Tamura |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2() |
Date: |
Tue, 25 Jan 2011 21:05:31 +0900 |
2011/1/25 Stefan Hajnoczi <address@hidden>:
> On Tue, Jan 25, 2011 at 10:13 AM, Corentin Chary
> <address@hidden> wrote:
>> On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <address@hidden> wrote:
>>> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
>>> <address@hidden> wrote:
>>>> From: Yoshiaki Tamura <address@hidden>
>>>>
>>>> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
>>>> deleting. This may cause a crash when a read handler calls
>>>> qemu_set_fd_handler2() to delete handlers, but a write handler is
>>>> still invoked from main_loop_wait(). Because main_loop_wait() checks
>>>> handlers before calling, setting NULL upon deleting will protect
>>>> handlers being called if already deleted.
>>>>
>>>> One example is the new threaded vnc server. When an error occurs in
>>>> the context of a read handler, it'll releases resources and deletes
>>>> handlers. However, because the write handler still exists, it'll be
>>>> called, and then crashes because of lack of resources. This patch
>>>> fixes it.
>>>
>>> Does this case still happen with qemu.git/master? In November I sent
>>> a patch to check for deleted handlers:
>>>
>>> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
>>> Author: Stefan Hajnoczi <address@hidden>
>>> Date: Wed Nov 3 14:29:44 2010 +0000
>>>
>>> Delete IOHandlers after potentially running them
>>>
>>> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>>> handler that deletes its IOHandler is exposed to .fd_write() being
>>> called on the deleted IOHandler.
>>>
>>> This patch fixes deletion so that .fd_read() and .fd_write() are never
>>> called on an IOHandler that is marked for deletion.
>>>
>>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>>> Signed-off-by: Anthony Liguori <address@hidden>
>>>
>>> So I don't think Yoshi's patch is necessary anymore?
>>
>> Ho I didn't see that one.
>> It's probably not necessary, but it stills make sense to apply this
>> patch since there is
>> absolutly no reasons to keep the old value in fd_read and fd_write when
>> the user explicitly asked to set them to NULL.
>
> That's true, I don't see a good reason why we shouldn't clear them.
> The only minor advantage to keeping them is that it helps when
> debugging QEMU - you can identify the fd handler by its
> fd_read/fd_write function pointers easily.
Well, since I posted the patch in August, I won't get surprised
even the bug might be fixed due to other patches :) Regardless of
that, I don't see the patch is doing something incorrect either.
If some codes were relying on unset values, that should be wrong.
Yoshi
>
> Stefan
>
>
- [Qemu-devel] [PATCH 2/7] Enable I/O thread and VNC threads by default, (continued)
[Qemu-devel] [PATCH 3/7] Add support for glib based threading and convert qemu thread to use it, Anthony Liguori, 2011/01/24
[Qemu-devel] [PATCH 7/7] Rename QemuThread to QemuSThread to indicate that it is not a generic thread, Anthony Liguori, 2011/01/24
[Qemu-devel] [PATCH 6/7] Teach vnc server to use GThread directly, Anthony Liguori, 2011/01/24
[Qemu-devel] [PATCH 4/7] Get rid of QemuMutex and teach its callers about GStaticMutex, Anthony Liguori, 2011/01/24