* [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg
@ 2011-09-07 12:30 Aneesh Kumar K.V
2011-09-07 18:44 ` Anthony Liguori
0 siblings, 1 reply; 6+ messages in thread
From: Aneesh Kumar K.V @ 2011-09-07 12:30 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, Aneesh Kumar K.V
Many users of qemu_fd_set_handler including VirtFS use NULL call back arg.
Update fd_trampoline and qemu_fd_set_handler to work with NULL call back arg
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
Changes from V1:
Add support for dropping event source
iohandler.c | 31 +++++++++++++------------------
1 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/iohandler.c b/iohandler.c
index 5ef66fb..783f3ac 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -93,9 +93,8 @@ static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer opaq
{
IOTrampoline *tramp = opaque;
- if (tramp->opaque == NULL) {
+ if (!tramp->fd_read && !tramp->fd_write)
return FALSE;
- }
if ((cond & G_IO_IN) && tramp->fd_read) {
tramp->fd_read(tramp->opaque);
@@ -113,6 +112,7 @@ int qemu_set_fd_handler(int fd,
IOHandler *fd_write,
void *opaque)
{
+ GIOCondition cond = 0;
static IOTrampoline fd_trampolines[FD_SETSIZE];
IOTrampoline *tramp = &fd_trampolines[fd];
@@ -121,25 +121,20 @@ int qemu_set_fd_handler(int fd,
g_source_remove(tramp->tag);
}
- if (opaque) {
- GIOCondition cond = 0;
-
- tramp->fd_read = fd_read;
- tramp->fd_write = fd_write;
- tramp->opaque = opaque;
-
- if (fd_read) {
- cond |= G_IO_IN | G_IO_ERR;
- }
-
- if (fd_write) {
- cond |= G_IO_OUT | G_IO_ERR;
- }
+ if (fd_read) {
+ cond |= G_IO_IN | G_IO_ERR;
+ }
- tramp->chan = g_io_channel_unix_new(fd);
- tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
+ if (fd_write) {
+ cond |= G_IO_OUT | G_IO_ERR;
}
+ tramp->fd_read = fd_read;
+ tramp->fd_write = fd_write;
+ tramp->opaque = opaque;
+ tramp->chan = g_io_channel_unix_new(fd);
+ tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
+
return 0;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg
2011-09-07 12:30 [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg Aneesh Kumar K.V
@ 2011-09-07 18:44 ` Anthony Liguori
2011-09-07 19:26 ` Aneesh Kumar K.V
2011-09-08 10:07 ` Avi Kivity
0 siblings, 2 replies; 6+ messages in thread
From: Anthony Liguori @ 2011-09-07 18:44 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Avi Kivity
On 09/07/2011 07:30 AM, Aneesh Kumar K.V wrote:
> Many users of qemu_fd_set_handler including VirtFS use NULL call back arg.
> Update fd_trampoline and qemu_fd_set_handler to work with NULL call back arg
>
> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> ---
> Changes from V1:
> Add support for dropping event source
>
> iohandler.c | 31 +++++++++++++------------------
> 1 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/iohandler.c b/iohandler.c
> index 5ef66fb..783f3ac 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -93,9 +93,8 @@ static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer opaq
> {
> IOTrampoline *tramp = opaque;
>
> - if (tramp->opaque == NULL) {
> + if (!tramp->fd_read&& !tramp->fd_write)
> return FALSE;
> - }
Heh, this is pretty anti-CODING_STYLE :-)
At any rate, I think this whole segment has to go. It's easier to not
deal with removing sources in two places.
>
> if ((cond& G_IO_IN)&& tramp->fd_read) {
> tramp->fd_read(tramp->opaque);
> @@ -113,6 +112,7 @@ int qemu_set_fd_handler(int fd,
> IOHandler *fd_write,
> void *opaque)
> {
> + GIOCondition cond = 0;
> static IOTrampoline fd_trampolines[FD_SETSIZE];
> IOTrampoline *tramp =&fd_trampolines[fd];
>
> @@ -121,25 +121,20 @@ int qemu_set_fd_handler(int fd,
> g_source_remove(tramp->tag);
> }
>
> - if (opaque) {
> - GIOCondition cond = 0;
> -
> - tramp->fd_read = fd_read;
> - tramp->fd_write = fd_write;
> - tramp->opaque = opaque;
> -
> - if (fd_read) {
> - cond |= G_IO_IN | G_IO_ERR;
> - }
> -
> - if (fd_write) {
> - cond |= G_IO_OUT | G_IO_ERR;
> - }
> + if (fd_read) {
> + cond |= G_IO_IN | G_IO_ERR;
> + }
>
> - tramp->chan = g_io_channel_unix_new(fd);
> - tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
> + if (fd_write) {
> + cond |= G_IO_OUT | G_IO_ERR;
> }
>
> + tramp->fd_read = fd_read;
> + tramp->fd_write = fd_write;
> + tramp->opaque = opaque;
> + tramp->chan = g_io_channel_unix_new(fd);
> + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
> +
I think this is a bit more complicated than is really needed. Here's
what I came up with which also fixes another bug where the io channel
could be freed twice. I stumbled across this via a very strange failure
scenario. Avi, it might be worth trying this patch to see if it fixes
your problem too.
One thing that I found challenging debugging this, coroutines make
valgrind very unhappy. Is it possible that we could have a command line
switch to fall back to the thread based coroutines so to make things
more valgrind friendly?
diff --git a/iohandler.c b/iohandler.c
index 5ef66fb..4cc1c5a 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -93,10 +93,6 @@ static gboolean fd_trampoline(GIOChannel *chan,
GIOCondition cond, gpointer opaq
{
IOTrampoline *tramp = opaque;
- if (tramp->opaque == NULL) {
- return FALSE;
- }
-
if ((cond & G_IO_IN) && tramp->fd_read) {
tramp->fd_read(tramp->opaque);
}
@@ -119,9 +115,10 @@ int qemu_set_fd_handler(int fd,
if (tramp->tag != 0) {
g_io_channel_unref(tramp->chan);
g_source_remove(tramp->tag);
+ tramp->tag = 0;
}
- if (opaque) {
+ if (fd_read || fd_write || opaque) {
GIOCondition cond = 0;
tramp->fd_read = fd_read;
Regards,
Anthony Liguori
> return 0;
> }
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg
2011-09-07 18:44 ` Anthony Liguori
@ 2011-09-07 19:26 ` Aneesh Kumar K.V
2011-09-08 10:07 ` Avi Kivity
1 sibling, 0 replies; 6+ messages in thread
From: Aneesh Kumar K.V @ 2011-09-07 19:26 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Avi Kivity
On Wed, 07 Sep 2011 13:44:39 -0500, Anthony Liguori <aliguori@us.ibm.com> wrote:
> On 09/07/2011 07:30 AM, Aneesh Kumar K.V wrote:
> > Many users of qemu_fd_set_handler including VirtFS use NULL call back arg.
> > Update fd_trampoline and qemu_fd_set_handler to work with NULL call back arg
> >
> > Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> > ---
> > Changes from V1:
> > Add support for dropping event source
> >
> > iohandler.c | 31 +++++++++++++------------------
> > 1 files changed, 13 insertions(+), 18 deletions(-)
> >
> > diff --git a/iohandler.c b/iohandler.c
> > index 5ef66fb..783f3ac 100644
> > --- a/iohandler.c
> > +++ b/iohandler.c
> > @@ -93,9 +93,8 @@ static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer opaq
> > {
> > IOTrampoline *tramp = opaque;
> >
> > - if (tramp->opaque == NULL) {
> > + if (!tramp->fd_read&& !tramp->fd_write)
> > return FALSE;
> > - }
>
> Heh, this is pretty anti-CODING_STYLE :-)
>
> At any rate, I think this whole segment has to go. It's easier to not
> deal with removing sources in two places.
>
> >
> > if ((cond& G_IO_IN)&& tramp->fd_read) {
> > tramp->fd_read(tramp->opaque);
> > @@ -113,6 +112,7 @@ int qemu_set_fd_handler(int fd,
> > IOHandler *fd_write,
> > void *opaque)
> > {
> > + GIOCondition cond = 0;
> > static IOTrampoline fd_trampolines[FD_SETSIZE];
> > IOTrampoline *tramp =&fd_trampolines[fd];
> >
> > @@ -121,25 +121,20 @@ int qemu_set_fd_handler(int fd,
> > g_source_remove(tramp->tag);
> > }
> >
> > - if (opaque) {
> > - GIOCondition cond = 0;
> > -
> > - tramp->fd_read = fd_read;
> > - tramp->fd_write = fd_write;
> > - tramp->opaque = opaque;
> > -
> > - if (fd_read) {
> > - cond |= G_IO_IN | G_IO_ERR;
> > - }
> > -
> > - if (fd_write) {
> > - cond |= G_IO_OUT | G_IO_ERR;
> > - }
> > + if (fd_read) {
> > + cond |= G_IO_IN | G_IO_ERR;
> > + }
> >
> > - tramp->chan = g_io_channel_unix_new(fd);
> > - tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
> > + if (fd_write) {
> > + cond |= G_IO_OUT | G_IO_ERR;
> > }
> >
> > + tramp->fd_read = fd_read;
> > + tramp->fd_write = fd_write;
> > + tramp->opaque = opaque;
> > + tramp->chan = g_io_channel_unix_new(fd);
> > + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
> > +
>
> I think this is a bit more complicated than is really needed. Here's
> what I came up with which also fixes another bug where the io channel
> could be freed twice. I stumbled across this via a very strange failure
> scenario. Avi, it might be worth trying this patch to see if it fixes
> your problem too.
>
> One thing that I found challenging debugging this, coroutines make
> valgrind very unhappy. Is it possible that we could have a command line
> switch to fall back to the thread based coroutines so to make things
> more valgrind friendly?
We definitely can look at a configure option that build with coroutine-gthread
>
> diff --git a/iohandler.c b/iohandler.c
> index 5ef66fb..4cc1c5a 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -93,10 +93,6 @@ static gboolean fd_trampoline(GIOChannel *chan,
> GIOCondition cond, gpointer opaq
> {
> IOTrampoline *tramp = opaque;
>
> - if (tramp->opaque == NULL) {
> - return FALSE;
> - }
> -
> if ((cond & G_IO_IN) && tramp->fd_read) {
> tramp->fd_read(tramp->opaque);
> }
> @@ -119,9 +115,10 @@ int qemu_set_fd_handler(int fd,
> if (tramp->tag != 0) {
> g_io_channel_unref(tramp->chan);
> g_source_remove(tramp->tag);
> + tramp->tag = 0;
> }
>
> - if (opaque) {
> + if (fd_read || fd_write || opaque) {
> GIOCondition cond = 0;
>
> tramp->fd_read = fd_read;
>
I tested above change with virtfs and it works.
-aneesh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg
2011-09-07 18:44 ` Anthony Liguori
2011-09-07 19:26 ` Aneesh Kumar K.V
@ 2011-09-08 10:07 ` Avi Kivity
2011-09-08 10:16 ` Kevin Wolf
2011-09-08 12:57 ` Anthony Liguori
1 sibling, 2 replies; 6+ messages in thread
From: Avi Kivity @ 2011-09-08 10:07 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Kevin Wolf, Aneesh Kumar K.V, Stefan Hajnoczi, qemu-devel
On 09/07/2011 09:44 PM, Anthony Liguori wrote:
>
> I think this is a bit more complicated than is really needed. Here's
> what I came up with which also fixes another bug where the io channel
> could be freed twice. I stumbled across this via a very strange
> failure scenario. Avi, it might be worth trying this patch to see if
> it fixes your problem too.
Right now, I've got more than just one problem.
>
> One thing that I found challenging debugging this, coroutines make
> valgrind very unhappy. Is it possible that we could have a command
> line switch to fall back to the thread based coroutines so to make
> things more valgrind friendly?
How is valgrind even aware of coroutines? Unless is doesn't implement
makecontext correctly, it shouldn't even be aware of them.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg
2011-09-08 10:07 ` Avi Kivity
@ 2011-09-08 10:16 ` Kevin Wolf
2011-09-08 12:57 ` Anthony Liguori
1 sibling, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2011-09-08 10:16 UTC (permalink / raw)
To: Avi Kivity; +Cc: Anthony Liguori, Aneesh Kumar K.V, Stefan Hajnoczi, qemu-devel
Am 08.09.2011 12:07, schrieb Avi Kivity:
> On 09/07/2011 09:44 PM, Anthony Liguori wrote:
>>
>> I think this is a bit more complicated than is really needed. Here's
>> what I came up with which also fixes another bug where the io channel
>> could be freed twice. I stumbled across this via a very strange
>> failure scenario. Avi, it might be worth trying this patch to see if
>> it fixes your problem too.
>
> Right now, I've got more than just one problem.
>
>>
>> One thing that I found challenging debugging this, coroutines make
>> valgrind very unhappy. Is it possible that we could have a command
>> line switch to fall back to the thread based coroutines so to make
>> things more valgrind friendly?
>
> How is valgrind even aware of coroutines? Unless is doesn't implement
> makecontext correctly, it shouldn't even be aware of them.
The F15 valgrind complains three times that the program is switching
stacks, but then it shuts up and just works as normal.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg
2011-09-08 10:07 ` Avi Kivity
2011-09-08 10:16 ` Kevin Wolf
@ 2011-09-08 12:57 ` Anthony Liguori
1 sibling, 0 replies; 6+ messages in thread
From: Anthony Liguori @ 2011-09-08 12:57 UTC (permalink / raw)
To: Avi Kivity; +Cc: Kevin Wolf, Aneesh Kumar K.V, Stefan Hajnoczi, qemu-devel
On 09/08/2011 05:07 AM, Avi Kivity wrote:
> On 09/07/2011 09:44 PM, Anthony Liguori wrote:
>>
>> I think this is a bit more complicated than is really needed. Here's
>> what I came up with which also fixes another bug where the io channel
>> could be freed twice. I stumbled across this via a very strange
>> failure scenario. Avi, it might be worth trying this patch to see if
>> it fixes your problem too.
>
> Right now, I've got more than just one problem.
>
>>
>> One thing that I found challenging debugging this, coroutines make
>> valgrind very unhappy. Is it possible that we could have a command
>> line switch to fall back to the thread based coroutines so to make
>> things more valgrind friendly?
>
> How is valgrind even aware of coroutines? Unless is doesn't implement
> makecontext correctly, it shouldn't even be aware of them.
It detects stack switching and has trouble differentiating between a
legitimate stack switch and something more nefarious. I believe the
heuristic it currently uses is the distance that RSP moves. If it moves
more than a certain threshold, it assumes that's a stack switch.
Regards,
Anthony Liguori
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-08 12:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-07 12:30 [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg Aneesh Kumar K.V
2011-09-07 18:44 ` Anthony Liguori
2011-09-07 19:26 ` Aneesh Kumar K.V
2011-09-08 10:07 ` Avi Kivity
2011-09-08 10:16 ` Kevin Wolf
2011-09-08 12:57 ` Anthony Liguori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).