[PATCH 1/2] Fix the segment fault when calling yank_register_instance

[PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Li Zhang]

From: Li Zhang <li.zhang@cloud.ionos.com>

When executing the QMP commands "chardev-change" to change the
backend device to socket, it will cause a segment fault because
it assumes chr->label as non-NULL in function yank_register_instance.
The function qmp_chardev_change calls chardev_new, which label
is NULL when creating a new chardev. The label will be passed to
yank_register_instance which causes a segment fault. The callchain
is as the following:
        chardev_new ->
            qemu_char_open ->
                cc->open ->
                qmp_chardev_open_socket ->
                    yank_register_instance

Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
---
 chardev/char-socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index c8bced76b7..26d5172682 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
         qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
     }
 
-    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
-        return;
+    if (chr->label) {
+        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+            return;
+        }
+        s->registered_yank = true;
     }
-    s->registered_yank = true;
 
     /* be isn't opened until we get a connection */
     *be_opened = false;
-- 
2.25.1

[PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

From: Li Zhang <li.zhang@cloud.ionos.com>

For some scenarios, it needs to hot-add a monitor device.
But QEMU doesn't support hotplug yet. It also works by adding
a monitor with null backend by default and then change its
backend to socket by QMP command "chardev-change".

So this patch is to support monitor chardev hotswap with QMP.

Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
---
 monitor/monitor-internal.h |  3 +++
 monitor/monitor.c          |  2 +-
 monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 40903d6386..2df6dd21de 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list);
 void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
                                  Error **errp);
 
+gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
+                               void *opaque);
+
 #endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index e94f532cf5..2d255bab18 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -157,7 +157,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
 
 static void monitor_flush_locked(Monitor *mon);
 
-static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
+gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
                                   void *opaque)
 {
     Monitor *mon = opaque;
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 2326bd7f9b..55cfb230d9 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -44,6 +44,7 @@ struct QMPRequest {
     Error *err;
 };
 typedef struct QMPRequest QMPRequest;
+static void monitor_qmp_set_handlers_bh(void *opaque);
 
 QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 
@@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
     g_queue_free(mon->qmp_requests);
 }
 
-static void monitor_qmp_setup_handlers_bh(void *opaque)
+static int monitor_qmp_change(void *opaque)
+{
+    MonitorQMP *mon = opaque;
+
+    mon->common.use_io_thread =
+        qemu_chr_has_feature(mon->common.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT);
+
+    if (mon->common.use_io_thread) {
+        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
+                                monitor_qmp_set_handlers_bh, mon);
+    } else {
+        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
+                                 monitor_qmp_read, monitor_qmp_event,
+                                 monitor_qmp_change, &mon->common, NULL, true);
+    }
+
+    if (mon->common.out_watch) {
+        g_source_remove(mon->common.out_watch);
+        qemu_mutex_lock(&mon->common.mon_lock);
+        mon->common.out_watch =
+        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
+                               monitor_unblocked, &mon->common);
+        qemu_mutex_unlock(&mon->common.mon_lock);
+    }
+
+    return 0;
+}
+
+static void monitor_qmp_set_handlers_bh(void *opaque)
 {
     MonitorQMP *mon = opaque;
     GMainContext *context;
@@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     assert(context);
     qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
                              monitor_qmp_read, monitor_qmp_event,
-                             NULL, &mon->common, context, true);
+                             monitor_qmp_change, &mon->common, context, true);
+
+}
+
+static void monitor_qmp_setup_handlers_bh(void *opaque)
+{
+    MonitorQMP *mon = opaque;
+    monitor_qmp_set_handlers_bh(mon);
     monitor_list_append(&mon->common);
 }
 
@@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
     } else {
         qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
                                  monitor_qmp_read, monitor_qmp_event,
-                                 NULL, &mon->common, NULL, true);
+                                 monitor_qmp_change, &mon->common, NULL, true);
         monitor_list_append(&mon->common);
     }
 }
-- 
2.25.1

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]

Hi,

Any comments about this patch?

Thanks
Li

On Mon, Mar 15, 2021 at 6:07 PM Li Zhang <zhlcindy@gmail.com> wrote:

> From: Li Zhang <li.zhang@cloud.ionos.com>
>
> For some scenarios, it needs to hot-add a monitor device.
> But QEMU doesn't support hotplug yet. It also works by adding
> a monitor with null backend by default and then change its
> backend to socket by QMP command "chardev-change".
>
> So this patch is to support monitor chardev hotswap with QMP.
>
> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> ---
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          |  2 +-
>  monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 40903d6386..2df6dd21de 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char
> *list);
>  void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
>                                   Error **errp);
>
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +                               void *opaque);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index e94f532cf5..2d255bab18 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -157,7 +157,7 @@ static inline bool
> monitor_is_hmp_non_interactive(const Monitor *mon)
>
>  static void monitor_flush_locked(Monitor *mon);
>
> -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>                                    void *opaque)
>  {
>      Monitor *mon = opaque;
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 2326bd7f9b..55cfb230d9 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -44,6 +44,7 @@ struct QMPRequest {
>      Error *err;
>  };
>  typedef struct QMPRequest QMPRequest;
> +static void monitor_qmp_set_handlers_bh(void *opaque);
>
>  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>
> @@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
>      g_queue_free(mon->qmp_requests);
>  }
>
> -static void monitor_qmp_setup_handlers_bh(void *opaque)
> +static int monitor_qmp_change(void *opaque)
> +{
> +    MonitorQMP *mon = opaque;
> +
> +    mon->common.use_io_thread =
> +        qemu_chr_has_feature(mon->common.chr.chr,
> QEMU_CHAR_FEATURE_GCONTEXT);
> +
> +    if (mon->common.use_io_thread) {
> +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +                                monitor_qmp_set_handlers_bh, mon);
> +    } else {
> +        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> +                                 monitor_qmp_read, monitor_qmp_event,
> +                                 monitor_qmp_change, &mon->common, NULL,
> true);
> +    }
> +
> +    if (mon->common.out_watch) {
> +        g_source_remove(mon->common.out_watch);
> +        qemu_mutex_lock(&mon->common.mon_lock);
> +        mon->common.out_watch =
> +        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> +                               monitor_unblocked, &mon->common);
> +        qemu_mutex_unlock(&mon->common.mon_lock);
> +    }
> +
> +    return 0;
> +}
> +
> +static void monitor_qmp_set_handlers_bh(void *opaque)
>  {
>      MonitorQMP *mon = opaque;
>      GMainContext *context;
> @@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void
> *opaque)
>      assert(context);
>      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                               monitor_qmp_read, monitor_qmp_event,
> -                             NULL, &mon->common, context, true);
> +                             monitor_qmp_change, &mon->common, context,
> true);
> +
> +}
> +
> +static void monitor_qmp_setup_handlers_bh(void *opaque)
> +{
> +    MonitorQMP *mon = opaque;
> +    monitor_qmp_set_handlers_bh(mon);
>      monitor_list_append(&mon->common);
>  }
>
> @@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error
> **errp)
>      } else {
>          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                                   monitor_qmp_read, monitor_qmp_event,
> -                                 NULL, &mon->common, NULL, true);
> +                                 monitor_qmp_change, &mon->common, NULL,
> true);
>          monitor_list_append(&mon->common);
>      }
>  }
> --
> 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 5860 bytes --]

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Markus Armbruster]

Li Zhang <li.zhang@ionos.com> writes:

> Hi,
>
> Any comments about this patch?

I wanted to review this before my Easter break, but I'm out of time :(

When I'm back (April 6), I'll check whether it still needs review, but I
do hope somebody else can look at it sooner.

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Fri, Mar 26, 2021 at 3:40 PM Markus Armbruster <armbru@redhat.com> wrote:

> Li Zhang <li.zhang@ionos.com> writes:
>
> > Hi,
> >
> > Any comments about this patch?
>
> I wanted to review this before my Easter break, but I'm out of time :(
>
> When I'm back (April 6), I'll check whether it still needs review, but I
> do hope somebody else can look at it sooner.
>
>
Thanks anyway. This patch is better to work with Lukas' patches together.

[-- Attachment #2: Type: text/html, Size: 947 bytes --]

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

Hi Markus,

Any suggestions on this patch?

Thanks
Li

On Fri, Mar 26, 2021 at 3:40 PM Markus Armbruster <armbru@redhat.com> wrote:

> Li Zhang <li.zhang@ionos.com> writes:
>
> > Hi,
> >
> > Any comments about this patch?
>
> I wanted to review this before my Easter break, but I'm out of time :(
>
> When I'm back (April 6), I'll check whether it still needs review, but I
> do hope somebody else can look at it sooner.
>
>

[-- Attachment #2: Type: text/html, Size: 882 bytes --]

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Markus Armbruster]

Li Zhang <li.zhang@ionos.com> writes:

> Hi Markus,
>
> Any suggestions on this patch?

I understand PATCH 1/2 got superseded by Lukas's "[PATCH v8 0/4] yank:
Add chardev tests and fixes".  I trust Marc-André will take care of it
in due time.

Before I look at the actual patch: does this patch depend on Lukas's fix
or your "[PATCH 1/2] Fix the segment fault when calling
yank_register_instance"?

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

Hi Markus,

[PATCH 1/2] Fix the segment fault when calling yank_register_instance is
reworked by Lukas.
And his patches have been merged to master branch of qemu.
[PATCH 2/2]  Support monitor chardev hotswap with QMP is to change monitor
backend with chardev-change. It is not implemented yet.


On Mon, Apr 12, 2021 at 3:10 PM Markus Armbruster <armbru@redhat.com> wrote:

> Li Zhang <li.zhang@ionos.com> writes:
>
> > Hi Markus,
> >
> > Any suggestions on this patch?
>
> I understand PATCH 1/2 got superseded by Lukas's "[PATCH v8 0/4] yank:
> Add chardev tests and fixes".  I trust Marc-André will take care of it
> in due time.
>
> Before I look at the actual patch: does this patch depend on Lukas's fix
> or your "[PATCH 1/2] Fix the segment fault when calling
> yank_register_instance"?
>
>

[-- Attachment #2: Type: text/html, Size: 1497 bytes --]

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Pankaj Gupta]

Hi Li,

> For some scenarios, it needs to hot-add a monitor device.
> But QEMU doesn't support hotplug yet. It also works by adding
> a monitor with null backend by default and then change its
> backend to socket by QMP command "chardev-change".
>
> So this patch is to support monitor chardev hotswap with QMP.
>
> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> ---
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          |  2 +-
>  monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 40903d6386..2df6dd21de 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list);
>  void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
>                                   Error **errp);
>
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +                               void *opaque);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index e94f532cf5..2d255bab18 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -157,7 +157,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
>
>  static void monitor_flush_locked(Monitor *mon);
>
> -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>                                    void *opaque)
>  {
>      Monitor *mon = opaque;
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 2326bd7f9b..55cfb230d9 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -44,6 +44,7 @@ struct QMPRequest {
>      Error *err;
>  };
>  typedef struct QMPRequest QMPRequest;
> +static void monitor_qmp_set_handlers_bh(void *opaque);
>
>  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>
> @@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
>      g_queue_free(mon->qmp_requests);
>  }
>
> -static void monitor_qmp_setup_handlers_bh(void *opaque)
> +static int monitor_qmp_change(void *opaque)
> +{
> +    MonitorQMP *mon = opaque;
> +
> +    mon->common.use_io_thread =
> +        qemu_chr_has_feature(mon->common.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT);
> +
> +    if (mon->common.use_io_thread) {
> +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +                                monitor_qmp_set_handlers_bh, mon);
> +    } else {
> +        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> +                                 monitor_qmp_read, monitor_qmp_event,
> +                                 monitor_qmp_change, &mon->common, NULL, true);
> +    }
> +
> +    if (mon->common.out_watch) {
> +        g_source_remove(mon->common.out_watch);
> +        qemu_mutex_lock(&mon->common.mon_lock);

Not very sure if mutex is must here, but still its protecting "out_watch"
for simultaneous updating.

> +        mon->common.out_watch =
> +        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> +                               monitor_unblocked, &mon->common);
> +        qemu_mutex_unlock(&mon->common.mon_lock);
> +    }
> +
> +    return 0;
> +}
> +
> +static void monitor_qmp_set_handlers_bh(void *opaque)
>  {
>      MonitorQMP *mon = opaque;
>      GMainContext *context;
> @@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
>      assert(context);
>      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                               monitor_qmp_read, monitor_qmp_event,
> -                             NULL, &mon->common, context, true);
> +                             monitor_qmp_change, &mon->common, context, true);
> +
> +}
> +
> +static void monitor_qmp_setup_handlers_bh(void *opaque)
> +{
> +    MonitorQMP *mon = opaque;
> +    monitor_qmp_set_handlers_bh(mon);
>      monitor_list_append(&mon->common);
>  }
>
> @@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>      } else {
>          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                                   monitor_qmp_read, monitor_qmp_event,
> -                                 NULL, &mon->common, NULL, true);
> +                                 monitor_qmp_change, &mon->common, NULL, true);
>          monitor_list_append(&mon->common);
>      }
>  }

Overall patch looks good to me.
Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Markus Armbruster]

Li Zhang <zhlcindy@gmail.com> writes:

> From: Li Zhang <li.zhang@cloud.ionos.com>
>
> For some scenarios, it needs to hot-add a monitor device.
> But QEMU doesn't support hotplug yet. It also works by adding
> a monitor with null backend by default and then change its
> backend to socket by QMP command "chardev-change".
>
> So this patch is to support monitor chardev hotswap with QMP.
>
> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>

I think what what you're trying to say is that chardev-change does not
work when the character device changes is used by a QMP monitor.
Correct?

If yes, how exactly does it misbehave?

Does it work with an HMP monitor?

> ---
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          |  2 +-
>  monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 40903d6386..2df6dd21de 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list);
>  void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
>                                   Error **errp);
>  
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +                               void *opaque);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index e94f532cf5..2d255bab18 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -157,7 +157,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
>  
>  static void monitor_flush_locked(Monitor *mon);
>  
> -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>                                    void *opaque)
>  {
>      Monitor *mon = opaque;
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 2326bd7f9b..55cfb230d9 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -44,6 +44,7 @@ struct QMPRequest {
>      Error *err;
>  };
>  typedef struct QMPRequest QMPRequest;
> +static void monitor_qmp_set_handlers_bh(void *opaque);
>  
>  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  
> @@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
>      g_queue_free(mon->qmp_requests);
>  }
>  
> -static void monitor_qmp_setup_handlers_bh(void *opaque)
> +static int monitor_qmp_change(void *opaque)
> +{
> +    MonitorQMP *mon = opaque;
> +
> +    mon->common.use_io_thread =
> +        qemu_chr_has_feature(mon->common.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT);
> +
> +    if (mon->common.use_io_thread) {
> +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +                                monitor_qmp_set_handlers_bh, mon);
> +    } else {
> +        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> +                                 monitor_qmp_read, monitor_qmp_event,
> +                                 monitor_qmp_change, &mon->common, NULL, true);
> +    }
> +
> +    if (mon->common.out_watch) {
> +        g_source_remove(mon->common.out_watch);

All other updates of @out_watch are under @mon_lock.  Why not this one?

I have no idea whether g_source_remove() is the right function to call.
Its documentation says "You must use g_source_destroy() for sources
added to a non-default main context."  The qemu_chr_fe_set_handlers()
contract is of no help.

Documentation of g_source_destroy() confuses some more: "This does not
unref the GSource: if you still hold a reference, use g_source_unref()
to drop it.

Marc-André, can you help?

> +        qemu_mutex_lock(&mon->common.mon_lock);
> +        mon->common.out_watch =
> +        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> +                               monitor_unblocked, &mon->common);

Bad indentation.  Better:

        mon->common.out_watch =
            qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
                                   monitor_unblocked, &mon->common);

or

        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
                                                      G_IO_OUT | G_IO_HUP,
                                                      monitor_unblocked,
                                                      &mon->common);

or

        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
                                        G_IO_OUT | G_IO_HUP,
                                        monitor_unblocked, &mon->common);

> +        qemu_mutex_unlock(&mon->common.mon_lock);
> +    }
> +
> +    return 0;
> +}

This function copies from monitor_data_init(), monitor_init_qmp(), and
monitor_flush_locked().  Feels like a refactoring would be in order.
Possibly on top.

> +
> +static void monitor_qmp_set_handlers_bh(void *opaque)
>  {
>      MonitorQMP *mon = opaque;
>      GMainContext *context;
> @@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
>      assert(context);
>      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                               monitor_qmp_read, monitor_qmp_event,
> -                             NULL, &mon->common, context, true);
> +                             monitor_qmp_change, &mon->common, context, true);
> +
> +}
> +
> +static void monitor_qmp_setup_handlers_bh(void *opaque)
> +{
> +    MonitorQMP *mon = opaque;
> +    monitor_qmp_set_handlers_bh(mon);
>      monitor_list_append(&mon->common);
>  }
>  
> @@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>      } else {
>          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                                   monitor_qmp_read, monitor_qmp_event,
> -                                 NULL, &mon->common, NULL, true);
> +                                 monitor_qmp_change, &mon->common, NULL, true);
>          monitor_list_append(&mon->common);
>      }
>  }

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

On Tue, Apr 13, 2021 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Li Zhang <zhlcindy@gmail.com> writes:
>
> > From: Li Zhang <li.zhang@cloud.ionos.com>
> >
> > For some scenarios, it needs to hot-add a monitor device.
> > But QEMU doesn't support hotplug yet. It also works by adding
> > a monitor with null backend by default and then change its
> > backend to socket by QMP command "chardev-change".
> >
> > So this patch is to support monitor chardev hotswap with QMP.
> >
> > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>
> I think what what you're trying to say is that chardev-change does not
> work when the character device changes is used by a QMP monitor.
> Correct?
>
I mean that when the character device is a monitor device, it doesn't
work with a QMP monitor.
For example, I add 2 QMP monitors and change one of the monitor's
backends from socket to a null device.
It doesn't work because it needs the monitor device to support chardev-change.

> If yes, how exactly does it misbehave?
This command chardev-change needs specific device's change callback function.

>
> Does it work with an HMP monitor?
No, it doesn't work.

>
> > ---
> >  monitor/monitor-internal.h |  3 +++
> >  monitor/monitor.c          |  2 +-
> >  monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
> >  3 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index 40903d6386..2df6dd21de 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list);
> >  void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
> >                                   Error **errp);
> >
> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +                               void *opaque);
> > +
> >  #endif
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index e94f532cf5..2d255bab18 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -157,7 +157,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> >
> >  static void monitor_flush_locked(Monitor *mon);
> >
> > -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> >                                    void *opaque)
> >  {
> >      Monitor *mon = opaque;
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 2326bd7f9b..55cfb230d9 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -44,6 +44,7 @@ struct QMPRequest {
> >      Error *err;
> >  };
> >  typedef struct QMPRequest QMPRequest;
> > +static void monitor_qmp_set_handlers_bh(void *opaque);
> >
> >  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> >
> > @@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
> >      g_queue_free(mon->qmp_requests);
> >  }
> >
> > -static void monitor_qmp_setup_handlers_bh(void *opaque)
> > +static int monitor_qmp_change(void *opaque)
> > +{
> > +    MonitorQMP *mon = opaque;
> > +
> > +    mon->common.use_io_thread =
> > +        qemu_chr_has_feature(mon->common.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT);
> > +
> > +    if (mon->common.use_io_thread) {
> > +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> > +                                monitor_qmp_set_handlers_bh, mon);
> > +    } else {
> > +        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> > +                                 monitor_qmp_read, monitor_qmp_event,
> > +                                 monitor_qmp_change, &mon->common, NULL, true);
> > +    }
> > +
> > +    if (mon->common.out_watch) {
> > +        g_source_remove(mon->common.out_watch);
>
> All other updates of @out_watch are under @mon_lock.  Why not this one?

Sorry, I missed it. I will correct it.

>
> I have no idea whether g_source_remove() is the right function to call.
> Its documentation says "You must use g_source_destroy() for sources
> added to a non-default main context."  The qemu_chr_fe_set_handlers()
> contract is of no help.
>
> Documentation of g_source_destroy() confuses some more: "This does not
> unref the GSource: if you still hold a reference, use g_source_unref()
> to drop it.
>
> Marc-André, can you help?
>
> > +        qemu_mutex_lock(&mon->common.mon_lock);
> > +        mon->common.out_watch =
> > +        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> > +                               monitor_unblocked, &mon->common);
>
> Bad indentation.  Better:
>
>         mon->common.out_watch =
>             qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
>                                    monitor_unblocked, &mon->common);
>
> or
>
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                                       G_IO_OUT | G_IO_HUP,
>                                                       monitor_unblocked,
>                                                       &mon->common);
>
> or
>
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                         G_IO_OUT | G_IO_HUP,
>                                         monitor_unblocked, &mon->common);
>
> > +        qemu_mutex_unlock(&mon->common.mon_lock);
> > +    }
> > +
> > +    return 0;
> > +}
>

I will correct it. Thanks.

> This function copies from monitor_data_init(), monitor_init_qmp(), and
> monitor_flush_locked().  Feels like a refactoring would be in order.
> Possibly on top.
>
OK, I am considering it to avoid duplicated code.

> > +
> > +static void monitor_qmp_set_handlers_bh(void *opaque)
> >  {
> >      MonitorQMP *mon = opaque;
> >      GMainContext *context;
> > @@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
> >      assert(context);
> >      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> >                               monitor_qmp_read, monitor_qmp_event,
> > -                             NULL, &mon->common, context, true);
> > +                             monitor_qmp_change, &mon->common, context, true);
> > +
> > +}
> > +
> > +static void monitor_qmp_setup_handlers_bh(void *opaque)
> > +{
> > +    MonitorQMP *mon = opaque;
> > +    monitor_qmp_set_handlers_bh(mon);
> >      monitor_list_append(&mon->common);
> >  }
> >
> > @@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> >      } else {
> >          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> >                                   monitor_qmp_read, monitor_qmp_event,
> > -                                 NULL, &mon->common, NULL, true);
> > +                                 monitor_qmp_change, &mon->common, NULL, true);
> >          monitor_list_append(&mon->common);
> >      }
> >  }
>


--

Best Regards
-Li

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Markus Armbruster]

Li Zhang <zhlcindy@gmail.com> writes:

> On Tue, Apr 13, 2021 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Li Zhang <zhlcindy@gmail.com> writes:
>>
>> > From: Li Zhang <li.zhang@cloud.ionos.com>
>> >
>> > For some scenarios, it needs to hot-add a monitor device.
>> > But QEMU doesn't support hotplug yet. It also works by adding
>> > a monitor with null backend by default and then change its
>> > backend to socket by QMP command "chardev-change".
>> >
>> > So this patch is to support monitor chardev hotswap with QMP.
>> >
>> > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>>
>> I think what what you're trying to say is that chardev-change does not
>> work when the character device changes is used by a QMP monitor.
>> Correct?
>>
> I mean that when the character device is a monitor device, it doesn't
> work with a QMP monitor.
> For example, I add 2 QMP monitors and change one of the monitor's
> backends from socket to a null device.
> It doesn't work because it needs the monitor device to support chardev-change.
>
>> If yes, how exactly does it misbehave?
> This command chardev-change needs specific device's change callback function.

Yes, but what happens if you try anyway?  I'm asking, because I'd like
the answer to be worked into the commit message.

>> Does it work with an HMP monitor?
> No, it doesn't work.

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

When I tried chardev-change, it reports this error:

{
    "error": {
        "class": "GenericError",
        "desc": "Chardev user does not support chardev hotswap"
    }
}

On Fri, Apr 16, 2021 at 11:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Li Zhang <zhlcindy@gmail.com> writes:
>
> > On Tue, Apr 13, 2021 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Li Zhang <zhlcindy@gmail.com> writes:
> >>
> >> > From: Li Zhang <li.zhang@cloud.ionos.com>
> >> >
> >> > For some scenarios, it needs to hot-add a monitor device.
> >> > But QEMU doesn't support hotplug yet. It also works by adding
> >> > a monitor with null backend by default and then change its
> >> > backend to socket by QMP command "chardev-change".
> >> >
> >> > So this patch is to support monitor chardev hotswap with QMP.
> >> >
> >> > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> >>
> >> I think what what you're trying to say is that chardev-change does not
> >> work when the character device changes is used by a QMP monitor.
> >> Correct?
> >>
> > I mean that when the character device is a monitor device, it doesn't
> > work with a QMP monitor.
> > For example, I add 2 QMP monitors and change one of the monitor's
> > backends from socket to a null device.
> > It doesn't work because it needs the monitor device to support chardev-change.
> >
> >> If yes, how exactly does it misbehave?
> > This command chardev-change needs specific device's change callback function.
>
> Yes, but what happens if you try anyway?  I'm asking, because I'd like
> the answer to be worked into the commit message.
>
> >> Does it work with an HMP monitor?
> > No, it doesn't work.
>


-- 

Best Regards
-Li

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Daniel P. Berrangé]

On Tue, Apr 13, 2021 at 08:40:59AM +0200, Markus Armbruster wrote:
> Li Zhang <zhlcindy@gmail.com> writes:
> 
> > From: Li Zhang <li.zhang@cloud.ionos.com>
> >
> > For some scenarios, it needs to hot-add a monitor device.
> > But QEMU doesn't support hotplug yet. It also works by adding
> > a monitor with null backend by default and then change its
> > backend to socket by QMP command "chardev-change".

If you need ability to hot-add monitor instances, why not just
implement that feature directly, instead of pre-creating monitors
with null backends and then later changing the backend ?

> >
> > So this patch is to support monitor chardev hotswap with QMP.
> >
> > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> 
> I think what what you're trying to say is that chardev-change does not
> work when the character device changes is used by a QMP monitor.
> Correct?
> 
> If yes, how exactly does it misbehave?
> 
> Does it work with an HMP monitor?
> 
> > ---
> >  monitor/monitor-internal.h |  3 +++
> >  monitor/monitor.c          |  2 +-
> >  monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
> >  3 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index 40903d6386..2df6dd21de 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list);
> >  void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
> >                                   Error **errp);
> >  
> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +                               void *opaque);
> > +
> >  #endif
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index e94f532cf5..2d255bab18 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -157,7 +157,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> >  
> >  static void monitor_flush_locked(Monitor *mon);
> >  
> > -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> >                                    void *opaque)
> >  {
> >      Monitor *mon = opaque;
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 2326bd7f9b..55cfb230d9 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -44,6 +44,7 @@ struct QMPRequest {
> >      Error *err;
> >  };
> >  typedef struct QMPRequest QMPRequest;
> > +static void monitor_qmp_set_handlers_bh(void *opaque);
> >  
> >  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> >  
> > @@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
> >      g_queue_free(mon->qmp_requests);
> >  }
> >  
> > -static void monitor_qmp_setup_handlers_bh(void *opaque)
> > +static int monitor_qmp_change(void *opaque)
> > +{
> > +    MonitorQMP *mon = opaque;
> > +
> > +    mon->common.use_io_thread =
> > +        qemu_chr_has_feature(mon->common.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT);
> > +
> > +    if (mon->common.use_io_thread) {
> > +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> > +                                monitor_qmp_set_handlers_bh, mon);
> > +    } else {
> > +        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> > +                                 monitor_qmp_read, monitor_qmp_event,
> > +                                 monitor_qmp_change, &mon->common, NULL, true);
> > +    }
> > +
> > +    if (mon->common.out_watch) {
> > +        g_source_remove(mon->common.out_watch);
> 
> All other updates of @out_watch are under @mon_lock.  Why not this one?
> 
> I have no idea whether g_source_remove() is the right function to call.
> Its documentation says "You must use g_source_destroy() for sources
> added to a non-default main context."  The qemu_chr_fe_set_handlers()
> contract is of no help.
> 
> Documentation of g_source_destroy() confuses some more: "This does not
> unref the GSource: if you still hold a reference, use g_source_unref()
> to drop it.
> 
> Marc-André, can you help?
> 
> > +        qemu_mutex_lock(&mon->common.mon_lock);
> > +        mon->common.out_watch =
> > +        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> > +                               monitor_unblocked, &mon->common);
> 
> Bad indentation.  Better:
> 
>         mon->common.out_watch =
>             qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
>                                    monitor_unblocked, &mon->common);
> 
> or
> 
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                                       G_IO_OUT | G_IO_HUP,
>                                                       monitor_unblocked,
>                                                       &mon->common);
> 
> or
> 
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                         G_IO_OUT | G_IO_HUP,
>                                         monitor_unblocked, &mon->common);
> 
> > +        qemu_mutex_unlock(&mon->common.mon_lock);
> > +    }
> > +
> > +    return 0;
> > +}
> 
> This function copies from monitor_data_init(), monitor_init_qmp(), and
> monitor_flush_locked().  Feels like a refactoring would be in order.
> Possibly on top.
> 
> > +
> > +static void monitor_qmp_set_handlers_bh(void *opaque)
> >  {
> >      MonitorQMP *mon = opaque;
> >      GMainContext *context;
> > @@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
> >      assert(context);
> >      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> >                               monitor_qmp_read, monitor_qmp_event,
> > -                             NULL, &mon->common, context, true);
> > +                             monitor_qmp_change, &mon->common, context, true);
> > +
> > +}
> > +
> > +static void monitor_qmp_setup_handlers_bh(void *opaque)
> > +{
> > +    MonitorQMP *mon = opaque;
> > +    monitor_qmp_set_handlers_bh(mon);
> >      monitor_list_append(&mon->common);
> >  }
> >  
> > @@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> >      } else {
> >          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> >                                   monitor_qmp_read, monitor_qmp_event,
> > -                                 NULL, &mon->common, NULL, true);
> > +                                 monitor_qmp_change, &mon->common, NULL, true);
> >          monitor_list_append(&mon->common);
> >      }
> >  }
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Li Zhang]

On Tue, Apr 13, 2021 at 10:58 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Apr 13, 2021 at 08:40:59AM +0200, Markus Armbruster wrote:
> > Li Zhang <zhlcindy@gmail.com> writes:
> >
> > > From: Li Zhang <li.zhang@cloud.ionos.com>
> > >
> > > For some scenarios, it needs to hot-add a monitor device.
> > > But QEMU doesn't support hotplug yet. It also works by adding
> > > a monitor with null backend by default and then change its
> > > backend to socket by QMP command "chardev-change".
>
> If you need ability to hot-add monitor instances, why not just
> implement that feature directly, instead of pre-creating monitors
> with null backends and then later changing the backend ?
>

Hi Daniel,

I would like to use this solution first because we'd like to use it as
soon as possible.
It's the best solution to implement the hot-add feature and it may
need more effort to do it.
I will consider it.

> > >
> > > So this patch is to support monitor chardev hotswap with QMP.
> > >
> > > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> >
> > I think what what you're trying to say is that chardev-change does not
> > work when the character device changes is used by a QMP monitor.
> > Correct?
> >
> > If yes, how exactly does it misbehave?
> >
> > Does it work with an HMP monitor?
> >
> > > ---
> > >  monitor/monitor-internal.h |  3 +++
> > >  monitor/monitor.c          |  2 +-
> > >  monitor/qmp.c              | 42 +++++++++++++++++++++++++++++++++++---
> > >  3 files changed, 43 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > > index 40903d6386..2df6dd21de 100644
> > > --- a/monitor/monitor-internal.h
> > > +++ b/monitor/monitor-internal.h
> > > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list);
> > >  void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
> > >                                   Error **errp);
> > >
> > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > > +                               void *opaque);
> > > +
> > >  #endif
> > > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > > index e94f532cf5..2d255bab18 100644
> > > --- a/monitor/monitor.c
> > > +++ b/monitor/monitor.c
> > > @@ -157,7 +157,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> > >
> > >  static void monitor_flush_locked(Monitor *mon);
> > >
> > > -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > >                                    void *opaque)
> > >  {
> > >      Monitor *mon = opaque;
> > > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > > index 2326bd7f9b..55cfb230d9 100644
> > > --- a/monitor/qmp.c
> > > +++ b/monitor/qmp.c
> > > @@ -44,6 +44,7 @@ struct QMPRequest {
> > >      Error *err;
> > >  };
> > >  typedef struct QMPRequest QMPRequest;
> > > +static void monitor_qmp_set_handlers_bh(void *opaque);
> > >
> > >  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> > >
> > > @@ -480,7 +481,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
> > >      g_queue_free(mon->qmp_requests);
> > >  }
> > >
> > > -static void monitor_qmp_setup_handlers_bh(void *opaque)
> > > +static int monitor_qmp_change(void *opaque)
> > > +{
> > > +    MonitorQMP *mon = opaque;
> > > +
> > > +    mon->common.use_io_thread =
> > > +        qemu_chr_has_feature(mon->common.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT);
> > > +
> > > +    if (mon->common.use_io_thread) {
> > > +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> > > +                                monitor_qmp_set_handlers_bh, mon);
> > > +    } else {
> > > +        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> > > +                                 monitor_qmp_read, monitor_qmp_event,
> > > +                                 monitor_qmp_change, &mon->common, NULL, true);
> > > +    }
> > > +
> > > +    if (mon->common.out_watch) {
> > > +        g_source_remove(mon->common.out_watch);
> >
> > All other updates of @out_watch are under @mon_lock.  Why not this one?
> >
> > I have no idea whether g_source_remove() is the right function to call.
> > Its documentation says "You must use g_source_destroy() for sources
> > added to a non-default main context."  The qemu_chr_fe_set_handlers()
> > contract is of no help.
> >
> > Documentation of g_source_destroy() confuses some more: "This does not
> > unref the GSource: if you still hold a reference, use g_source_unref()
> > to drop it.
> >
> > Marc-André, can you help?
> >
> > > +        qemu_mutex_lock(&mon->common.mon_lock);
> > > +        mon->common.out_watch =
> > > +        qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> > > +                               monitor_unblocked, &mon->common);
> >
> > Bad indentation.  Better:
> >
> >         mon->common.out_watch =
> >             qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP,
> >                                    monitor_unblocked, &mon->common);
> >
> > or
> >
> >         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
> >                                                       G_IO_OUT | G_IO_HUP,
> >                                                       monitor_unblocked,
> >                                                       &mon->common);
> >
> > or
> >
> >         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
> >                                         G_IO_OUT | G_IO_HUP,
> >                                         monitor_unblocked, &mon->common);
> >
> > > +        qemu_mutex_unlock(&mon->common.mon_lock);
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> >
> > This function copies from monitor_data_init(), monitor_init_qmp(), and
> > monitor_flush_locked().  Feels like a refactoring would be in order.
> > Possibly on top.
> >
> > > +
> > > +static void monitor_qmp_set_handlers_bh(void *opaque)
> > >  {
> > >      MonitorQMP *mon = opaque;
> > >      GMainContext *context;
> > > @@ -490,7 +519,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
> > >      assert(context);
> > >      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> > >                               monitor_qmp_read, monitor_qmp_event,
> > > -                             NULL, &mon->common, context, true);
> > > +                             monitor_qmp_change, &mon->common, context, true);
> > > +
> > > +}
> > > +
> > > +static void monitor_qmp_setup_handlers_bh(void *opaque)
> > > +{
> > > +    MonitorQMP *mon = opaque;
> > > +    monitor_qmp_set_handlers_bh(mon);
> > >      monitor_list_append(&mon->common);
> > >  }
> > >
> > > @@ -531,7 +567,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> > >      } else {
> > >          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
> > >                                   monitor_qmp_read, monitor_qmp_event,
> > > -                                 NULL, &mon->common, NULL, true);
> > > +                                 monitor_qmp_change, &mon->common, NULL, true);
> > >          monitor_list_append(&mon->common);
> > >      }
> > >  }
> >
> >
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>


-- 

Best Regards
-Li

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

Hi

On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:

> From: Li Zhang <li.zhang@cloud.ionos.com>
>
> When executing the QMP commands "chardev-change" to change the
> backend device to socket, it will cause a segment fault because
> it assumes chr->label as non-NULL in function yank_register_instance.
> The function qmp_chardev_change calls chardev_new, which label
> is NULL when creating a new chardev. The label will be passed to
> yank_register_instance which causes a segment fault. The callchain
> is as the following:
>         chardev_new ->
>             qemu_char_open ->
>                 cc->open ->
>                 qmp_chardev_open_socket ->
>                     yank_register_instance
>
> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> ---
>  chardev/char-socket.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index c8bced76b7..26d5172682 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>      }
>
> -    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp))
> {
> -        return;
> +    if (chr->label) {
> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
> errp)) {
> +            return;
> +        }
> +        s->registered_yank = true;
>      }
> -    s->registered_yank = true;
>
>      /* be isn't opened until we get a connection */
>      *be_opened = false
>

Looks wrong to me, the new chardev will get the same label, and it should
still be possible to call the yank functions then. The registration logic
needs to be reworked during chardev-change.

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2596 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]

Hi Marc-André,

The new chardev can get the same label. It is assigned after the function

ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                                  Error **errp)
{
     .....
     chr_new = chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
                          backend, chr->gcontext, errp);
    if (!chr_new) {
        return NULL;
    }
    chr_new->label = g_strdup(id);
    if (chr->be_open && !chr_new->be_open) {
        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
        closed_sent = true;
    }

    chr->be = NULL;
    qemu_chr_fe_init(be, chr_new, &error_abort);
       .....
}

It passes parameter NULL in chardev_new, I think it may be because the old
chardev isn't released yet.
It will cause duplicated problems. I need to consider this logic to see if
it can be changed.

Thanks
Li


On Mon, Mar 15, 2021 at 7:51 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
>> From: Li Zhang <li.zhang@cloud.ionos.com>
>>
>> When executing the QMP commands "chardev-change" to change the
>> backend device to socket, it will cause a segment fault because
>> it assumes chr->label as non-NULL in function yank_register_instance.
>> The function qmp_chardev_change calls chardev_new, which label
>> is NULL when creating a new chardev. The label will be passed to
>> yank_register_instance which causes a segment fault. The callchain
>> is as the following:
>>         chardev_new ->
>>             qemu_char_open ->
>>                 cc->open ->
>>                 qmp_chardev_open_socket ->
>>                     yank_register_instance
>>
>> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>> ---
>>  chardev/char-socket.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index c8bced76b7..26d5172682 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>>      }
>>
>> -    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>> errp)) {
>> -        return;
>> +    if (chr->label) {
>> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>> errp)) {
>> +            return;
>> +        }
>> +        s->registered_yank = true;
>>      }
>> -    s->registered_yank = true;
>>
>>      /* be isn't opened until we get a connection */
>>      *be_opened = false
>>
>
> Looks wrong to me, the new chardev will get the same label, and it should
> still be possible to call the yank functions then. The registration logic
> needs to be reworked during chardev-change.
>
> --
> Marc-André Lureau
>

[-- Attachment #2: Type: text/html, Size: 4161 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]

Hi Marc-André,

By looking into chardev and yank_register_function logic,  this old chardev
is registered according to the chardev label.
So it's been in yank_instance_list. yank instance only has a chardev label,
and the new chardev's label is the same as the old chardev.
So it doesn't need to register it again when changing the chardev backend.
Otherwise, it will report  duplicated yank instances.
I think the chardev logic has no problems. And it works with yank
functions.

Thanks
Li

On Mon, Mar 15, 2021 at 7:51 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
>> From: Li Zhang <li.zhang@cloud.ionos.com>
>>
>> When executing the QMP commands "chardev-change" to change the
>> backend device to socket, it will cause a segment fault because
>> it assumes chr->label as non-NULL in function yank_register_instance.
>> The function qmp_chardev_change calls chardev_new, which label
>> is NULL when creating a new chardev. The label will be passed to
>> yank_register_instance which causes a segment fault. The callchain
>> is as the following:
>>         chardev_new ->
>>             qemu_char_open ->
>>                 cc->open ->
>>                 qmp_chardev_open_socket ->
>>                     yank_register_instance
>>
>> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>> ---
>>  chardev/char-socket.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index c8bced76b7..26d5172682 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>>      }
>>
>> -    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>> errp)) {
>> -        return;
>> +    if (chr->label) {
>> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>> errp)) {
>> +            return;
>> +        }
>> +        s->registered_yank = true;
>>      }
>> -    s->registered_yank = true;
>>
>>      /* be isn't opened until we get a connection */
>>      *be_opened = false
>>
>
> Looks wrong to me, the new chardev will get the same label, and it should
> still be possible to call the yank functions then. The registration logic
> needs to be reworked during chardev-change.
>
> --
> Marc-André Lureau
>

[-- Attachment #2: Type: text/html, Size: 3569 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2955 bytes --]

Hi

On Tue, Mar 16, 2021 at 6:46 PM Li Zhang <li.zhang@cloud.ionos.com> wrote:

> Hi Marc-André,
>
> By looking into chardev and yank_register_function logic,  this old
> chardev is registered according to the chardev label.
> So it's been in yank_instance_list. yank instance only has a chardev
> label, and the new chardev's label is the same as the old chardev.
> So it doesn't need to register it again when changing the chardev backend.
> Otherwise, it will report  duplicated yank instances.
> I think the chardev logic has no problems. And it works with yank
> functions.
>
>
The previous instance is being removed with yank_unregister_instance()
during object_unparent(). The new instance is not registered.

That scenario deserves tests. (it's a shame there are no tests for yank ...)



> Thanks
> Li
>
> On Mon, Mar 15, 2021 at 7:51 PM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:
>>
>>> From: Li Zhang <li.zhang@cloud.ionos.com>
>>>
>>> When executing the QMP commands "chardev-change" to change the
>>> backend device to socket, it will cause a segment fault because
>>> it assumes chr->label as non-NULL in function yank_register_instance.
>>> The function qmp_chardev_change calls chardev_new, which label
>>> is NULL when creating a new chardev. The label will be passed to
>>> yank_register_instance which causes a segment fault. The callchain
>>> is as the following:
>>>         chardev_new ->
>>>             qemu_char_open ->
>>>                 cc->open ->
>>>                 qmp_chardev_open_socket ->
>>>                     yank_register_instance
>>>
>>> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>>> ---
>>>  chardev/char-socket.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index c8bced76b7..26d5172682 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>>>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>>>      }
>>>
>>> -    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>>> errp)) {
>>> -        return;
>>> +    if (chr->label) {
>>> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>>> errp)) {
>>> +            return;
>>> +        }
>>> +        s->registered_yank = true;
>>>      }
>>> -    s->registered_yank = true;
>>>
>>>      /* be isn't opened until we get a connection */
>>>      *be_opened = false
>>>
>>
>> Looks wrong to me, the new chardev will get the same label, and it should
>> still be possible to call the yank functions then. The registration logic
>> needs to be reworked during chardev-change.
>>
>> --
>> Marc-André Lureau
>>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4530 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Li Zhang]

[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]

Hi Marc-André,

Hi Marc-André,

Ah, you are right.  For some scenarios, it is not registered and registered
for some scenarios.
If the previous chardev is not socket, it won't be registered either.
There are still problems.

On Tue, Mar 16, 2021 at 4:25 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Tue, Mar 16, 2021 at 6:46 PM Li Zhang <li.zhang@cloud.ionos.com> wrote:
>
>> Hi Marc-André,
>>
>> By looking into chardev and yank_register_function logic,  this old
>> chardev is registered according to the chardev label.
>> So it's been in yank_instance_list. yank instance only has a chardev
>> label, and the new chardev's label is the same as the old chardev.
>> So it doesn't need to register it again when changing the chardev
>> backend. Otherwise, it will report  duplicated yank instances.
>> I think the chardev logic has no problems. And it works with yank
>> functions.
>>
>>
> The previous instance is being removed with yank_unregister_instance()
> during object_unparent(). The new instance is not registered.
>
> That scenario deserves tests. (it's a shame there are no tests for yank
> ...)
>
>
>
>> Thanks
>> Li
>>
>> On Mon, Mar 15, 2021 at 7:51 PM Marc-André Lureau <
>> marcandre.lureau@gmail.com> wrote:
>>
>>> Hi
>>>
>>> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:
>>>
>>>> From: Li Zhang <li.zhang@cloud.ionos.com>
>>>>
>>>> When executing the QMP commands "chardev-change" to change the
>>>> backend device to socket, it will cause a segment fault because
>>>> it assumes chr->label as non-NULL in function yank_register_instance.
>>>> The function qmp_chardev_change calls chardev_new, which label
>>>> is NULL when creating a new chardev. The label will be passed to
>>>> yank_register_instance which causes a segment fault. The callchain
>>>> is as the following:
>>>>         chardev_new ->
>>>>             qemu_char_open ->
>>>>                 cc->open ->
>>>>                 qmp_chardev_open_socket ->
>>>>                     yank_register_instance
>>>>
>>>> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>>>> ---
>>>>  chardev/char-socket.c | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>>> index c8bced76b7..26d5172682 100644
>>>> --- a/chardev/char-socket.c
>>>> +++ b/chardev/char-socket.c
>>>> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev
>>>> *chr,
>>>>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>>>>      }
>>>>
>>>> -    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>>>> errp)) {
>>>> -        return;
>>>> +    if (chr->label) {
>>>> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>>>> errp)) {
>>>> +            return;
>>>> +        }
>>>> +        s->registered_yank = true;
>>>>      }
>>>> -    s->registered_yank = true;
>>>>
>>>>      /* be isn't opened until we get a connection */
>>>>      *be_opened = false
>>>>
>>>
>>> Looks wrong to me, the new chardev will get the same label, and it
>>> should still be possible to call the yank functions then. The registration
>>> logic needs to be reworked during chardev-change.
>>>
>>> --
>>> Marc-André Lureau
>>>
>>
>
> --
> Marc-André Lureau
>

[-- Attachment #2: Type: text/html, Size: 5216 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Mon, 15 Mar 2021 18:06:35 +0100
Li Zhang <zhlcindy@gmail.com> wrote:

> From: Li Zhang <li.zhang@cloud.ionos.com>
> 
> When executing the QMP commands "chardev-change" to change the
> backend device to socket, it will cause a segment fault because
> it assumes chr->label as non-NULL in function yank_register_instance.
> The function qmp_chardev_change calls chardev_new, which label
> is NULL when creating a new chardev. The label will be passed to
> yank_register_instance which causes a segment fault. The callchain
> is as the following:
>         chardev_new ->
>             qemu_char_open ->
>                 cc->open ->
>                 qmp_chardev_open_socket ->
>                     yank_register_instance

Oh, I didn't consider the chardev-change case. I'll look into it.

Regards,
Lukas Straub

-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Markus Armbruster]

Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?

Li Zhang <zhlcindy@gmail.com> writes:

> From: Li Zhang <li.zhang@cloud.ionos.com>
>
> When executing the QMP commands "chardev-change" to change the
> backend device to socket, it will cause a segment fault because
> it assumes chr->label as non-NULL in function yank_register_instance.
> The function qmp_chardev_change calls chardev_new, which label
> is NULL when creating a new chardev. The label will be passed to
> yank_register_instance which causes a segment fault. The callchain
> is as the following:
>         chardev_new ->
>             qemu_char_open ->
>                 cc->open ->
>                 qmp_chardev_open_socket ->
>                     yank_register_instance
>
> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
> ---
>  chardev/char-socket.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index c8bced76b7..26d5172682 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>          qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>      }
>  
> -    if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
> -        return;
> +    if (chr->label) {
> +        if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
> +            return;
> +        }
> +        s->registered_yank = true;
>      }
> -    s->registered_yank = true;
>  
>      /* be isn't opened until we get a connection */
>      *be_opened = false;

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Fri, 26 Mar 2021 15:41:11 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?
> 

Yeah, this patch only fixes a symptom, but not the core cause of the bug.
I have already written patches that fix the bugs inclusive test-cases:
https://lore.kernel.org/qemu-devel/cover.1616744509.git.lukasstraub2@web.de/

Regards,
Lukas Straub

-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Markus Armbruster]

Lukas Straub <lukasstraub2@web.de> writes:

> On Fri, 26 Mar 2021 15:41:11 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?
>> 
>
> Yeah, this patch only fixes a symptom, but not the core cause of the bug.
> I have already written patches that fix the bugs inclusive test-cases:
> https://lore.kernel.org/qemu-devel/cover.1616744509.git.lukasstraub2@web.de/

Awesome!  I didn't make the connection.  I trust you'll take care of
getting the fixes merged :)