On Thu, Jan 04, 2018 at 10:31:58AM +0800, Peter Xu wrote:
> On Wed, Jan 03, 2018 at 05:41:53PM +0000, Stefan Hajnoczi wrote:
> > On Wed, Jan 03, 2018 at 10:24:18AM +0800, Peter Xu 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>
> > > ---
> > >
> > > v2 -> v2.1: Sorry I forgot to do the move in char.h. Did it in this
> > > minor version.
> > >
> > > chardev/char-pty.c | 9 ++-------
> > > chardev/char-socket.c | 4 ++--
> > > chardev/char.c | 20 ++++++++++++++++++++
> > > hw/char/terminal3270.c | 7 ++++---
> > > include/chardev/char.h | 3 +++
> > > 5 files changed, 31 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);
> >
> > The label is user-visible. Why did you remove the seconds label format?
>
> It's used for g_source_set_name_by_id() below, and that's not
> user-visible AFAICT?
>
> I removed it because I thought it was not user visible and actually I
> didn't see a point on doing that. Please let me know if I made a
> mistake.
>
> >
> > Please either include justification in the commit description or avoid
> > spurious changes like this so reviewers don't need to worry about code
> > changes that are not essential.
>
> Yes. I can add this into commit message after confirmed with you on above.You are right, the GSource name isn't visible (it's only used for debugging). I was thinking of chr->label. It's still helpful to mention that the ms == 1000 special case is not user-visible in the commit description. Thanks, Stefan
signature.asc
Description: PGP signature
