On Tue, Jan 02, 2018 at 05:10:01PM +0100, Marc-André Lureau wrote:
> Hi
>
> On Thu, Dec 28, 2017 at 8:29 AM, Peter Xu <pet...@redhat.com> wrote:
> > It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs
> > now can have dedicated gcontext, we should always bind chardev tasks
> > onto those gcontext rather than the default main context. Since there
> > are quite a few of g_timeout_add[_seconds]() callers, a new function
> > qemu_chr_timeout_add() is introduced.
> >
> > One thing to mention is that, terminal3270 is still always running on
> > main gcontext. However let's convert that as well since it's still part
> > of chardev codes and in case one day we'll miss that when we move it out
> > of main gcontext too.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > chardev/char-pty.c | 9 ++-------
> > chardev/char-socket.c | 4 ++--
> > chardev/char.c | 20 ++++++++++++++++++++
> > hw/char/terminal3270.c | 7 ++++---
> > include/chardev/char.h | 2 ++
> > 5 files changed, 30 insertions(+), 12 deletions(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index dd17b1b823..cbd8ac5eb7 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
> > s->timer_tag = 0;
> > }
> >
> > - if (ms == 1000) {
> > - name = g_strdup_printf("pty-timer-secs-%s", chr->label);
> > - s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
> > - } else {
> > - name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> > - s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
> > - }
> > + name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> > + s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
> > g_source_set_name_by_id(s->timer_tag, name);
> > g_free(name);
> > }
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 630a7f2995..644a620599 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
> > char *name;
> >
> > assert(s->connected == 0);
> > - s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
> > - socket_reconnect_timeout,
> > chr);
> > + s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time,
>
> reconnect_time is in second, you should * 1000.
Yes. Will fix them all.
>
> > + socket_reconnect_timeout,
> > chr);
> > name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
> > g_source_set_name_by_id(s->reconnect_timer, name);
> > g_free(name);
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 8c3765ee99..a1de662fec 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error
> > **errp)
> > qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> > }
> >
> > +/*
> > + * Add a timeout callback for the chardev (in milliseconds). Please
> > + * use this to add timeout hook for chardev instead of g_timeout_add()
> > + * and g_timeout_add_seconds(), to make sure the gcontext that the
> > + * task bound to is correct.
> > + */
> > +guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
> > + void *private)
> > +{
> > + GSource *source = g_timeout_source_new(ms);
> > + guint id;
> > +
> > + assert(func);
> > + g_source_set_callback(source, func, private, NULL);
> > + id = g_source_attach(source, chr->gcontext);
> > + g_source_unref(source);
> > +
> > + return id;
> > +}
> > +
> > void qemu_chr_cleanup(void)
> > {
> > object_unparent(get_chardevs_root());
> > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> > index a109ce5987..479e4554d6 100644
> > --- a/hw/char/terminal3270.c
> > +++ b/hw/char/terminal3270.c
> > @@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t
> > *buf, int size)
> > g_source_remove(t->timer_tag);
> > t->timer_tag = 0;
> > }
> > - t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
> > -
> > + t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600,
>
> same here
>
> > + send_timing_mark_cb, t);
> > memcpy(&t->inv[t->in_len], buf, size);
> > t->in_len += size;
> > if (t->in_len < 2) {
> > @@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)
> > * char-socket.c. Once qemu receives the terminal-type of the
> > * client, mark handshake done and trigger everything rolling
> > again.
> > */
> > - t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
> > + t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600,
>
> and again
>
> > + send_timing_mark_cb, t);
> > break;
> > case CHR_EVENT_CLOSED:
> > sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> > index 778d610295..4970e46457 100644
> > --- a/include/chardev/char.h
> > +++ b/include/chardev/char.h
> > @@ -258,5 +258,7 @@ extern int term_escape_char;
> >
> > /* console.c */
> > void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error
> > **errp);
> > +guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
> > + void *private);
> >
>
> Please put the declaration before the /* console.c */ comment.
Sure. Thanks for review.
--
Peter Xu
