Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

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 <peterx@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]