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

[Markus Armbruster]

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 capability. Otherwise, print an
> error (arguably, it may assert instead).

Really?  I think you do assert instead.  Hmm, misunderstanding; this
isn't about what the patch does, it's about what callers should do.
Perhaps:

  QEMU_CHAR_FEATURE_GCONTEXT declares the character device can switch
  GMainContext.

  Assert we don't switch context when the character device doesn't
  provide this feature.  Character device users must not violate this
  restriction.  In particular, user configurations that violate them
  must be rejected.

This leads me to my next question: is it currently possible to violate
this restriction and trip the new assertion?

>
> 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 152dde5327..123b566cba 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) {
> +        g_assert_not_reached();
>      }

Hmm.

cc->chr_update_read_handler is true iff QEMU_CHAR_FEATURE_GCONTEXT.
Therefore, the assertion really asserts "context may be non-null only
when we have QEMU_CHAR_FEATURE_GCONTEXT".  Let's make that clearer:

       assert(qemu_chr_has_feature(s, QEMU_CHAR_FEATURE_GCONTEXT)
              || !context);
       s->gcontext = context;
       if (cc->chr_update_read_handler) {
           cc->chr_update_read_handler(s);
       }

>  }
>  
> @@ -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)