Re: [Qemu-devel] [PATCH v2 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Markus Armbruster]

This one needs review by a chardev guy, with an eye on its use in the
next patch.  Paolo?

Marc-André Lureau <address@hidden> writes:

> The feature should be set if the chardev is able to switch
> GMainContext. Callers that want to put a chardev in a different thread
> context can/should check this capabilities.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/chardev/char.h |  3 +++
>  chardev/char.c         | 11 +++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 7becd8c80c..014566c3de 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -47,6 +47,9 @@ typedef enum {
>      QEMU_CHAR_FEATURE_FD_PASS,
>      /* Whether replay or record mode is enabled */
>      QEMU_CHAR_FEATURE_REPLAY,
> +    /* Whether the gcontext can be changed after calling
> +     * qemu_chr_be_update_read_handlers() */
> +    QEMU_CHAR_FEATURE_GCONTEXT,
>  
>      QEMU_CHAR_FEATURE_LAST,
>  } ChardevFeature;
> diff --git a/chardev/char.c b/chardev/char.c
> index 7f07a1bfbd..b5ee58b7d2 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
>      s->gcontext = context;
>      if (cc->chr_update_read_handler) {
>          cc->chr_update_read_handler(s);
> +    } else if (s->gcontext) {
> +        error_report("switching context isn't supported by this chardev");

Code smell: error_report() without returning failure.

If it's truly an error, then the function should fail.  We'd still need
to decide how: error_report() and return -1, or error_setg().

Except if it's a programming error; then we should assert().

If it's not an error, it's probably a warning, and we should use
warn_report().

>      }
>  }
>  
> @@ -240,6 +242,15 @@ static void char_init(Object *obj)
>  
>      chr->logfd = -1;
>      qemu_mutex_init(&chr->chr_write_lock);
> +
> +    /*
> +     * Assume if chr_update_read_handler is implemented it will
> +     * take the updated gcontext into account.
> +     */
> +    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
> +    }
> +
>  }
>  
>  static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)