Re: [Qemu-devel] [PATCH v2 3/4] char: remove the right fd been watched in qemu_chr_fe_set_handlers()

[Hailiang Zhang]

On 2017/2/16 21:04, Marc-André Lureau wrote:
> Hi
>
> On Thu, Feb 16, 2017 at 4:49 PM Hailiang Zhang <
> address@hidden> wrote:
>
> > Hi,
> >
> > On 2017/2/16 18:40, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Thu, Feb 16, 2017 at 10:08 AM zhanghailiang <
> > > address@hidden> wrote:
> > >
> > > > We can call qemu_chr_fe_set_handlers() to add/remove fd been watched
> > > > in 'context' which can be either default main context or other explicit
> > > > context. But the original logic is not correct, we didn't remove
> > > > the right fd because we call g_main_context_find_source_by_id(NULL, tag)
> > > > which always try to find the Gsource from default context.
> > > >
> > > > Fix it by passing the right context to
> > g_main_context_find_source_by_id().
> > > > Cc: Paolo Bonzini <address@hidden>
> > > > Cc: Marc-André Lureau <address@hidden>
> > > > Signed-off-by: zhanghailiang <address@hidden>
> > > > ---
> > > >    chardev/char-io.c | 13 +++++++++----
> > > >    chardev/char-io.h |  2 ++
> > > >    chardev/char.c    |  2 +-
> > > >    3 files changed, 12 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/chardev/char-io.c b/chardev/char-io.c
> > > > index 7dfc3f2..a69cc61 100644
> > > > --- a/chardev/char-io.c
> > > > +++ b/chardev/char-io.c
> > > > @@ -127,14 +127,14 @@ guint io_add_watch_poll(Chardev *chr,
> > > >        return tag;
> > > >    }
> > > >
> > > > -static void io_remove_watch_poll(guint tag)
> > > > +static void io_remove_watch_poll(guint tag, GMainContext *context)
> > > >    {
> > > >        GSource *source;
> > > >        IOWatchPoll *iwp;
> > > >
> > > >        g_return_if_fail(tag > 0);
> > > >
> > > > -    source = g_main_context_find_source_by_id(NULL, tag);
> > > > +    source = g_main_context_find_source_by_id(context, tag);
> > > >        g_return_if_fail(source != NULL);
> > > >
> > > >        iwp = io_watch_poll_from_source(source);
> > > > @@ -146,14 +146,19 @@ static void io_remove_watch_poll(guint tag)
> > > >        g_source_destroy(&iwp->parent);
> > > >    }
> > > >
> > > > -void remove_fd_in_watch(Chardev *chr)
> > > > +void qemu_remove_fd_in_watch(Chardev *chr, GMainContext *context)
> > > >    {
> > > >        if (chr->fd_in_tag) {
> > > > -        io_remove_watch_poll(chr->fd_in_tag);
> > > > +        io_remove_watch_poll(chr->fd_in_tag, context);
> > > >            chr->fd_in_tag = 0;
> > > >        }
> > > >    }
> > > >
> > > > +void remove_fd_in_watch(Chardev *chr)
> > > > +{
> > > > +    qemu_remove_fd_in_watch(chr, NULL);
> > > > +}
> > > > +
> > >
> > > I would just change the signature of remove_fd_in_watch() instead of
> > > introducing a function.
> >
> > In that case, i have to modify all the places call this helper,
> > About eleven places in six files，is it acceptable ？
>
> Yes, that's fine.

OK, will fix it in v3, thanks.

> > Thanks.
> >
> > >    int io_channel_send_full(QIOChannel *ioc,
> > > >                             const void *buf, size_t len,
> > > >                             int *fds, size_t nfds)
> > > > diff --git a/chardev/char-io.h b/chardev/char-io.h
> > > > index d7ae5f1..117c888 100644
> > > > --- a/chardev/char-io.h
> > > > +++ b/chardev/char-io.h
> > > > @@ -38,6 +38,8 @@ guint io_add_watch_poll(Chardev *chr,
> > > >
> > > >    void remove_fd_in_watch(Chardev *chr);
> > > >
> > > > +void qemu_remove_fd_in_watch(Chardev *chr, GMainContext *context);
> > > > +
> > > >    int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
> > > >
> > > >    int io_channel_send_full(QIOChannel *ioc, const void *buf, size_t len,
> > > > diff --git a/chardev/char.c b/chardev/char.c
> > > > index abd525f..5563375 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);
> > > > +        qemu_remove_fd_in_watch(s, context);
> > > >        } else {
> > > >            fe_open = 1;
> > > >        }
> > > > --
> > > > 1.8.3.1
> > > otherwise, looks good to me
> >
> > --
> Marc-André Lureau