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

[Marc-André Lureau]

Hi

On Mon, Dec 3, 2018 at 11:25 AM Markus Armbruster <address@hidden> wrote:
>
> 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().
>

After this patch, it's an error.
There should be a preliminary check for qemu_chr_has_feature(chr,
QEMU_CHAR_FEATURE_GCONTEXT) before switching context.

I'll replace the error_report() with an 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)
>


-- 
Marc-André Lureau