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

[Stefan Hajnoczi <stefanha@gmail.com>]

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

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?

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.

>      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..5cca32f963 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);

Here it was clear that reconnect_time is in seconds...

> +    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time * 1000,
> +                                              socket_reconnect_timeout, chr);

...now I can't tell what the unit is.

Please rename qemu_chr_timeout_add() to include the units:

  s->reconnect_timer = qemu_chr_timeout_add_ms(chr, s->reconnect_time * 1000,

>      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.
> + */

What is the return value?

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