Re: [Qemu-devel] [PULL 08/51] chardev: introduce qemu_chr_timeout_add_ms()

[Marc-André Lureau]

Hi

On Tue, Jan 16, 2018 at 3:16 PM, Paolo Bonzini <address@hidden> wrote:
> From: Peter Xu <address@hidden>
>
> 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_ms() 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.
>
> Also, convert all the timers from GSource tags into GSource pointers.
> Gsource tag IDs and g_source_remove()s can only work with default
> gcontext, while now these GSources can logically be attached to other
> contexts.  So let's use explicit g_source_destroy() plus another
> g_source_unref() to remove a timer.
>
> Note: when in the timer handler, we don't need the g_source_destroy()
> any more since that'll be done automatically if the timer handler
> returns false (and that's what all the current handlers do).
>
> Yet another note: in pty_chr_rearm_timer() we take special care for
> ms=1000.  This patch merged the two cases into one.
>
> Signed-off-by: Peter Xu <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  chardev/char-pty.c     | 43 +++++++++++++++++++------------------------
>  chardev/char-socket.c  | 28 ++++++++++++++++++----------
>  chardev/char.c         | 18 ++++++++++++++++++
>  hw/char/terminal3270.c | 28 ++++++++++++++++------------
>  include/chardev/char.h |  3 +++
>  5 files changed, 74 insertions(+), 46 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 8248e36..89315e6 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -42,7 +42,7 @@ typedef struct {
>
>      /* Protected by the Chardev chr_write_lock.  */
>      int connected;
> -    guint timer_tag;
> +    GSource *timer_src;
>      GSource *open_source;
>  } PtyChardev;
>
> @@ -57,7 +57,8 @@ static gboolean pty_chr_timer(gpointer opaque)
>      PtyChardev *s = PTY_CHARDEV(opaque);
>
>      qemu_mutex_lock(&chr->chr_write_lock);
> -    s->timer_tag = 0;
> +    s->timer_src = NULL;
> +    g_source_unref(s->open_source);

why that line ^ ? It adds criticals every second (for ex with -chardev
pty,id=foo -device isa-serial,chardev=foo).



-- 
Marc-André Lureau