| Message ID | 1492141282-28708-1-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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? > > 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). > > 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. > } > } > > 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 > > >
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 >> >> >> > . >
On 04/14/2017 05:10 AM, 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. 6 chars is indeed short, 7 is git's default as usually long enough, although I've encountered collisions that require 8 chars. [And google has proved that you can have a collision across the entire hash, although that is harder to generate.] I generally use 8 or so when writing commit messages. Fortunately, even if a collision is introduces later, someone that is motivated enough can still resolve the collision by filtering out any collisions that resolve to non-commits, and among the remaining colliding SHA1 focus on the one that has a commit date which predates the message with the reference.
On 2017/4/18 21:36, Eric Blake wrote: > On 04/14/2017 05:10 AM, 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. > 6 chars is indeed short, 7 is git's default as usually long enough, > although I've encountered collisions that require 8 chars. [And google > has proved that you can have a collision across the entire hash, Thanks for your explanation. I didn't notice that before, I have been always using the 6 chars hash to get the commit ... > although that is harder to generate.] I generally use 8 or so when > writing commit messages. Fortunately, even if a collision is introduces > later, someone that is motivated enough can still resolve the collision > by filtering out any collisions that resolve to non-commits, and among > the remaining colliding SHA1 focus on the one that has a commit date > which predates the message with the reference. Agreed, but I'd better to update the comment to make it clearer. thanks.
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; } } 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; };
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 fix the bug completely, because we still have some codes didn't pass *right* context parameter for remove_fd_in_watch(). Let's fix it by record the GSource directly instead of fd_in_tag. 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(-)