Re: [Qemu-devel] [PATCH] char: Fix removing wrong GSource that be found by fd_in_tag

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

Hi,

On 2017/4/14 18:10, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> We use fd_in_tag to find a GSource, fd_in_tag is return value of
>> g_source_attach(GSource *source, GMainContext *context), the return
>> value is unique only in the same context, so we may get the same
>> values with different 'context' parameters.
>>
>> It is no problem to find the right fd_in_tag by using
>>   g_main_context_find_source_by_id(GMainContext *context, guint source_id)
>> while there is only one default main context.
>>
>> But colo-compare tries to create/use its own context, and if we pass wrong
>> 'context' parameter with right fd_in_tag, we will find a wrong GSource
>> to handle. We tied to fix the related codes in commit b43dec, but it didn't
> tied->tried
>
> Please use a bit longer commit sha1, or full sha1, it will likely conflict otherwise in the future.

OK, I'll replace it with the full sha1.
>> fix the bug completely, because we still have some codes didn't pass *right*
>> context parameter for remove_fd_in_watch().
> Mixing contexts sounds like a colo-compare bug, did you fix it there too?

Yes, we don't have to change any other codes in colo-compare,
We just call qemu_chr_fe_set_handlers() in colo-compare to detach the old GSource
from main default context, and attach the new GSource to the new context.

>> Let's fix it by record the GSource directly instead of fd_in_tag.
> Make sense to me, and even simplify a bit the code. We should be more careful with pointers though (the reason why tag existed in the first place I imagine).

Agreed.

>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   chardev/char-fd.c     |  8 ++++----
>>   chardev/char-io.c     | 23 ++++++++---------------
>>   chardev/char-io.h     |  4 ++--
>>   chardev/char-pty.c    |  6 +++---
>>   chardev/char-socket.c |  8 ++++----
>>   chardev/char-udp.c    |  8 ++++----
>>   chardev/char.c        |  2 +-
>>   include/sysemu/char.h |  2 +-
>>   8 files changed, 27 insertions(+), 34 deletions(-)
>>
>> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
>> index 548dd4c..7f0169d 100644
>> --- a/chardev/char-fd.c
>> +++ b/chardev/char-fd.c
>> @@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition
>> cond, void *opaque)
>>       ret = qio_channel_read(
>>           chan, (gchar *)buf, len, NULL);
>>       if (ret == 0) {
>> -        remove_fd_in_watch(chr, NULL);
>> +        remove_fd_in_watch(chr);
>>           qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>>           return FALSE;
>>       }
>> @@ -89,9 +89,9 @@ static void fd_chr_update_read_handler(Chardev *chr,
>>   {
>>       FDChardev *s = FD_CHARDEV(chr);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc_in) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc_in,
>>                                              fd_chr_read_poll,
>>                                              fd_chr_read, chr,
>>                                              context);
>> @@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
>>       Chardev *chr = CHARDEV(obj);
>>       FDChardev *s = FD_CHARDEV(obj);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc_in) {
>>           object_unref(OBJECT(s->ioc_in));
>>       }
>> diff --git a/chardev/char-io.c b/chardev/char-io.c
>> index b4bb094..6deb193 100644
>> --- a/chardev/char-io.c
>> +++ b/chardev/char-io.c
>> @@ -98,7 +98,7 @@ static GSourceFuncs io_watch_poll_funcs = {
>>       .finalize = io_watch_poll_finalize,
>>   };
>>   
>> -guint io_add_watch_poll(Chardev *chr,
>> +GSource *io_add_watch_poll(Chardev *chr,
>>                           QIOChannel *ioc,
>>                           IOCanReadHandler *fd_can_read,
>>                           QIOChannelFunc fd_read,
>> @@ -106,7 +106,6 @@ guint io_add_watch_poll(Chardev *chr,
>>                           GMainContext *context)
>>   {
>>       IOWatchPoll *iwp;
>> -    int tag;
>>       char *name;
>>   
>>       iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs,
>> @@ -122,21 +121,15 @@ guint io_add_watch_poll(Chardev *chr,
>>       g_source_set_name((GSource *)iwp, name);
>>       g_free(name);
>>   
>> -    tag = g_source_attach(&iwp->parent, context);
>> +    g_source_attach(&iwp->parent, context);
>>       g_source_unref(&iwp->parent);
>> -    return tag;
>> +    return (GSource *)iwp;
>>   }
>>   
>> -static void io_remove_watch_poll(guint tag, GMainContext *context)
>> +static void io_remove_watch_poll(GSource *source)
>>   {
>> -    GSource *source;
>>       IOWatchPoll *iwp;
>>   
>> -    g_return_if_fail(tag > 0);
>> -
>> -    source = g_main_context_find_source_by_id(context, tag);
>> -    g_return_if_fail(source != NULL);
>> -
>>       iwp = io_watch_poll_from_source(source);
>>       if (iwp->src) {
>>           g_source_destroy(iwp->src);
>> @@ -146,11 +139,11 @@ static void io_remove_watch_poll(guint tag,
>> GMainContext *context)
>>       g_source_destroy(&iwp->parent);
>>   }
>>   
>> -void remove_fd_in_watch(Chardev *chr, GMainContext *context)
>> +void remove_fd_in_watch(Chardev *chr)
>>   {
>> -    if (chr->fd_in_tag) {
>> -        io_remove_watch_poll(chr->fd_in_tag, context);
>> -        chr->fd_in_tag = 0;
>> +    if (chr->chr_gsource) {
>> +        io_remove_watch_poll(chr->chr_gsource);
>> +        chr->chr_gsource = 0;
> It's a pointer, let's use NULL.

OK. Will fix in next version.

Thanks,
Hailiang
>>       }
>>   }
>>   
>> diff --git a/chardev/char-io.h b/chardev/char-io.h
>> index 842be56..55973a7 100644
>> --- a/chardev/char-io.h
>> +++ b/chardev/char-io.h
>> @@ -29,14 +29,14 @@
>>   #include "sysemu/char.h"
>>   
>>   /* Can only be used for read */
>> -guint io_add_watch_poll(Chardev *chr,
>> +GSource *io_add_watch_poll(Chardev *chr,
>>                           QIOChannel *ioc,
>>                           IOCanReadHandler *fd_can_read,
>>                           QIOChannelFunc fd_read,
>>                           gpointer user_data,
>>                           GMainContext *context);
>>   
>> -void remove_fd_in_watch(Chardev *chr, GMainContext *context);
>> +void remove_fd_in_watch(Chardev *chr);
>>   
>>   int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
>>   
>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> index a6337be..561926f 100644
>> --- a/chardev/char-pty.c
>> +++ b/chardev/char-pty.c
>> @@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>>               g_source_remove(s->open_tag);
>>               s->open_tag = 0;
>>           }
>> -        remove_fd_in_watch(chr, NULL);
>> +        remove_fd_in_watch(chr);
>>           s->connected = 0;
>>           /* (re-)connect poll interval for idle guests: once per second.
>>            * We check more frequently in case the guests sends data to
>> @@ -215,8 +215,8 @@ static void pty_chr_state(Chardev *chr, int connected)
>>               s->connected = 1;
>>               s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
>>           }
>> -        if (!chr->fd_in_tag) {
>> -            chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        if (!chr->chr_gsource) {
>> +            chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                                  pty_chr_read_poll,
>>                                                  pty_chr_read,
>>                                                  chr, NULL);
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index 36ab0d6..376a70b 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -327,7 +327,7 @@ static void tcp_chr_free_connection(Chardev *chr)
>>       }
>>   
>>       tcp_set_msgfds(chr, NULL, 0);
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       object_unref(OBJECT(s->sioc));
>>       s->sioc = NULL;
>>       object_unref(OBJECT(s->ioc));
>> @@ -484,7 +484,7 @@ static void tcp_chr_connect(void *opaque)
>>   
>>       s->connected = 1;
>>       if (s->ioc) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                              tcp_chr_read_poll,
>>                                              tcp_chr_read,
>>                                              chr, NULL);
>> @@ -501,9 +501,9 @@ static void tcp_chr_update_read_handler(Chardev *chr,
>>           return;
>>       }
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                              tcp_chr_read_poll,
>>                                              tcp_chr_read, chr,
>>                                              context);
>> diff --git a/chardev/char-udp.c b/chardev/char-udp.c
>> index 804bd22..9d050bc 100644
>> --- a/chardev/char-udp.c
>> +++ b/chardev/char-udp.c
>> @@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition
>> cond, void *opaque)
>>       ret = qio_channel_read(
>>           s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
>>       if (ret <= 0) {
>> -        remove_fd_in_watch(chr, NULL);
>> +        remove_fd_in_watch(chr);
>>           return FALSE;
>>       }
>>       s->bufcnt = ret;
>> @@ -101,9 +101,9 @@ static void udp_chr_update_read_handler(Chardev *chr,
>>   {
>>       UdpChardev *s = UDP_CHARDEV(chr);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                              udp_chr_read_poll,
>>                                              udp_chr_read, chr,
>>                                              context);
>> @@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
>>       Chardev *chr = CHARDEV(obj);
>>       UdpChardev *s = UDP_CHARDEV(obj);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc) {
>>           object_unref(OBJECT(s->ioc));
>>       }
>> diff --git a/chardev/char.c b/chardev/char.c
>> index 3df1163..54cd5f4 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>>       cc = CHARDEV_GET_CLASS(s);
>>       if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>>           fe_open = 0;
>> -        remove_fd_in_watch(s, context);
>> +        remove_fd_in_watch(s);
>>       } else {
>>           fe_open = 1;
>>       }
>> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> index 450881d..e157f96 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -93,7 +93,7 @@ struct Chardev {
>>       char *filename;
>>       int logfd;
>>       int be_open;
>> -    guint fd_in_tag;
>> +    GSource *chr_gsource;
>>       DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
>>       QTAILQ_ENTRY(Chardev) next;
>>   };
>> --
>> 1.8.3.1
>>
>>
>>
> .
>