| Message ID | 1487224821-120916-4-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi On Thu, Feb 16, 2017 at 10:08 AM zhanghailiang < zhang.zhanghailiang@huawei.com> 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 <pbonzini@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > 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. 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
Hi, On 2017/2/16 18:40, Marc-André Lureau wrote: > Hi > > On Thu, Feb 16, 2017 at 10:08 AM zhanghailiang < > zhang.zhanghailiang@huawei.com> 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 <pbonzini@redhat.com> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> --- >> 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 ？ 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 >
Hi On Thu, Feb 16, 2017 at 4:49 PM Hailiang Zhang < zhang.zhanghailiang@huawei.com> wrote: > Hi, > > On 2017/2/16 18:40, Marc-André Lureau wrote: > > Hi > > > > On Thu, Feb 16, 2017 at 10:08 AM zhanghailiang < > > zhang.zhanghailiang@huawei.com> 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 <pbonzini@redhat.com> > >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > >> --- > >> 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. > > 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
On 2017/2/16 21:04, Marc-André Lureau wrote: > Hi > > On Thu, Feb 16, 2017 at 4:49 PM Hailiang Zhang < > zhang.zhanghailiang@huawei.com> wrote: > >> Hi, >> >> On 2017/2/16 18:40, Marc-André Lureau wrote: >>> Hi >>> >>> On Thu, Feb 16, 2017 at 10:08 AM zhanghailiang < >>> zhang.zhanghailiang@huawei.com> 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 <pbonzini@redhat.com> >>>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> --- >>>> 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 >
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); +} + 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; }
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 <pbonzini@redhat.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- chardev/char-io.c | 13 +++++++++---- chardev/char-io.h | 2 ++ chardev/char.c | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-)