[v2] monitor: misc fixes

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

Posted by Marc-André Lureau 7 years ago

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 <marcandre.lureau@redhat.com>
---
 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");
     }
 }
 
@@ -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)
-- 
2.19.0.271.gfe8321ec05

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

Posted by Markus Armbruster 6 years, 11 months ago

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

Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> ---
>  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)

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

Posted by Marc-André Lureau 6 years, 11 months ago

Hi

On Mon, Dec 3, 2018 at 11:25 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> This one needs review by a chardev guy, with an eye on its use in the
> next patch.  Paolo?
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> > ---
> >  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

[Qemu-devel] [PATCH v2 1/6] monitor: inline ambiguous helper functions
[Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread
[Qemu-devel] [PATCH v2 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag
[Qemu-devel] [PATCH v2 4/6] monitor: check if chardev can switch gcontext for OOB
[Qemu-devel] [PATCH v2 5/6] monitor: prevent inserting new monitors after cleanup
[Qemu-devel] [PATCH v2 6/6] monitor: avoid potential dead-lock when cleaning up