Re: [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> g_source_attach can return any value between 1 and UINT_MAX if you let
> QEMU run long enough.  However, qemu_chr_fe_add_watch can also return
> a negative errno value when the device is disconnected or does not
> support chr_add_watch.  Change it to return zero to avoid overloading
> these values.
> 
> Fix the cadence_uart which asserts in this case (easily obtained with
> "-serial pty").
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/char/cadence_uart.c |  5 ++++-
>  include/sysemu/char.h  | 16 ++++++++++++++--
>  qemu-char.c            |  8 ++++----
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index c856fc3..488a570 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -294,7 +294,10 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, 
> GIOCondition cond,
>      if (s->tx_count) {
>          int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
>                                        cadence_uart_xmit, s);
> -        assert(r);
> +        if (!r) {
> +            s->tx_count = 0;
> +            return FALSE;
> +        }
>      }
>  
>      uart_update_status(s);
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 1eb2d0f..07434a0 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -221,8 +221,20 @@ void qemu_chr_fe_event(CharDriverState *s, int event);
>  void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  
> -int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> -                          GIOFunc func, void *user_data);
> +/**
> + * @qemu_chr_fe_add_watch:
> + *
> + * If the backend is connected, create and add a #GSource that fires
> + * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
> + * is active; return the #GSource's tag.  If it is disconnected,
> + * return 0.
> + *
> + * @cond the condition to poll for
> + * @func the function to call when the condition happens
> + * @user_data the opaque pointer to pass to @func
> + */
> +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> +                               GIOFunc func, void *user_data);

can we make this a guint?  hw/char/virtio-console.c and monitor.c already
use guint as the type for a watch and that matches the glib definition?
(Although not net/vhost-user.c which still has int, but that should probably
be fixed anyway).

But other than that; 

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Dave

>  
>  /**
>   * @qemu_chr_fe_write:
> diff --git a/qemu-char.c b/qemu-char.c
> index 84f49ac..39b2ccd 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3966,19 +3966,19 @@ void qemu_chr_fe_event(struct CharDriverState *chr, 
> int event)
>      }
>  }
>  
> -int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> -                          GIOFunc func, void *user_data)
> +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> +                               GIOFunc func, void *user_data)
>  {
>      GSource *src;
>      guint tag;
>  
>      if (s->chr_add_watch == NULL) {
> -        return -ENOSYS;
> +        return 0;
>      }
>  
>      src = s->chr_add_watch(s, cond);
>      if (!src) {
> -        return -EINVAL;
> +        return 0;
>      }
>  
>      g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
> -- 
> 2.5.5
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK