qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] Fix the segment fault when calling yank_register_instance
@ 2021-03-15 17:06 Li Zhang
  2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Li Zhang @ 2021-03-15 17:06 UTC (permalink / raw)
  To: marcandre.lureau, lukasstraub2, armbru
  Cc: Li Zhang, pankaj.gupta, alexandr.iarygin, qemu-devel

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



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-15 17:06 [PATCH 1/2] Fix the segment fault when calling yank_register_instance Li Zhang
@ 2021-03-15 17:06 ` Li Zhang
  2021-03-23 13:14   ` Li Zhang
                     ` (2 more replies)
  2021-03-15 18:51 ` [PATCH 1/2] Fix the segment fault when calling yank_register_instance Marc-André Lureau
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 24+ messages in thread
From: Li Zhang @ 2021-03-15 17:06 UTC (permalink / raw)
  To: marcandre.lureau, lukasstraub2, armbru
  Cc: Li Zhang, pankaj.gupta, alexandr.iarygin, qemu-devel

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



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-15 17:06 [PATCH 1/2] Fix the segment fault when calling yank_register_instance Li Zhang
  2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
@ 2021-03-15 18:51 ` Marc-André Lureau
  2021-03-16  9:45   ` Li Zhang
  2021-03-16 14:46   ` Li Zhang
  2021-03-17 21:06 ` Lukas Straub
  2021-03-26 14:41 ` Markus Armbruster
  3 siblings, 2 replies; 24+ messages in thread
From: Marc-André Lureau @ 2021-03-15 18:51 UTC (permalink / raw)
  To: Li Zhang
  Cc: Lukas Straub, alexandr.iarygin, Markus Armbruster, QEMU,
	Li Zhang, pankaj.gupta

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-15 18:51 ` [PATCH 1/2] Fix the segment fault when calling yank_register_instance Marc-André Lureau
@ 2021-03-16  9:45   ` Li Zhang
  2021-03-16 14:46   ` Li Zhang
  1 sibling, 0 replies; 24+ messages in thread
From: Li Zhang @ 2021-03-16  9:45 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Lukas Straub, alexandr.iarygin, Markus Armbruster, QEMU,
	Pankaj Gupta, 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-15 18:51 ` [PATCH 1/2] Fix the segment fault when calling yank_register_instance Marc-André Lureau
  2021-03-16  9:45   ` Li Zhang
@ 2021-03-16 14:46   ` Li Zhang
  2021-03-16 15:24     ` Marc-André Lureau
  1 sibling, 1 reply; 24+ messages in thread
From: Li Zhang @ 2021-03-16 14:46 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Lukas Straub, alexandr.iarygin, Markus Armbruster, QEMU,
	Pankaj Gupta, 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-16 14:46   ` Li Zhang
@ 2021-03-16 15:24     ` Marc-André Lureau
  2021-03-16 15:36       ` Li Zhang
  0 siblings, 1 reply; 24+ messages in thread
From: Marc-André Lureau @ 2021-03-16 15:24 UTC (permalink / raw)
  To: Li Zhang
  Cc: Lukas Straub, alexandr.iarygin, Markus Armbruster, QEMU,
	Pankaj Gupta, Li Zhang

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-16 15:24     ` Marc-André Lureau
@ 2021-03-16 15:36       ` Li Zhang
  0 siblings, 0 replies; 24+ messages in thread
From: Li Zhang @ 2021-03-16 15:36 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Lukas Straub, alexandr.iarygin, Markus Armbruster, QEMU,
	Pankaj Gupta, 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-15 17:06 [PATCH 1/2] Fix the segment fault when calling yank_register_instance Li Zhang
  2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
  2021-03-15 18:51 ` [PATCH 1/2] Fix the segment fault when calling yank_register_instance Marc-André Lureau
@ 2021-03-17 21:06 ` Lukas Straub
  2021-03-26 14:41 ` Markus Armbruster
  3 siblings, 0 replies; 24+ messages in thread
From: Lukas Straub @ 2021-03-17 21:06 UTC (permalink / raw)
  To: Li Zhang
  Cc: alexandr.iarygin, armbru, qemu-devel, Li Zhang, pankaj.gupta,
	marcandre.lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
@ 2021-03-23 13:14   ` Li Zhang
  2021-03-26 14:40     ` Markus Armbruster
  2021-04-12 14:19   ` Pankaj Gupta
  2021-04-13  6:40   ` Markus Armbruster
  2 siblings, 1 reply; 24+ messages in thread
From: Li Zhang @ 2021-03-23 13:14 UTC (permalink / raw)
  To: Li Zhang
  Cc: Lukas Straub, alexandr.iarygin, Markus Armbruster, QEMU,
	Li Zhang, Pankaj Gupta, marcandre.lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-23 13:14   ` Li Zhang
@ 2021-03-26 14:40     ` Markus Armbruster
  2021-03-26 15:19       ` Li Zhang
  2021-04-12 12:41       ` Li Zhang
  0 siblings, 2 replies; 24+ messages in thread
From: Markus Armbruster @ 2021-03-26 14:40 UTC (permalink / raw)
  To: Li Zhang
  Cc: Lukas Straub, alexandr.iarygin, QEMU, Li Zhang, Pankaj Gupta,
	Li Zhang, marcandre.lureau

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.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-15 17:06 [PATCH 1/2] Fix the segment fault when calling yank_register_instance Li Zhang
                   ` (2 preceding siblings ...)
  2021-03-17 21:06 ` Lukas Straub
@ 2021-03-26 14:41 ` Markus Armbruster
  2021-03-26 16:02   ` Lukas Straub
  3 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2021-03-26 14:41 UTC (permalink / raw)
  To: lukasstraub2
  Cc: alexandr.iarygin, qemu-devel, Li Zhang, pankaj.gupta, Li Zhang,
	marcandre.lureau

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;



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-26 14:40     ` Markus Armbruster
@ 2021-03-26 15:19       ` Li Zhang
  2021-04-12 12:41       ` Li Zhang
  1 sibling, 0 replies; 24+ messages in thread
From: Li Zhang @ 2021-03-26 15:19 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, alexandr.iarygin, QEMU, Li Zhang, Pankaj Gupta,
	Li Zhang, marcandre.lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-26 14:41 ` Markus Armbruster
@ 2021-03-26 16:02   ` Lukas Straub
  2021-03-26 16:13     ` Markus Armbruster
  0 siblings, 1 reply; 24+ messages in thread
From: Lukas Straub @ 2021-03-26 16:02 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: alexandr.iarygin, qemu-devel, Li Zhang, pankaj.gupta, Li Zhang,
	marcandre.lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
  2021-03-26 16:02   ` Lukas Straub
@ 2021-03-26 16:13     ` Markus Armbruster
  0 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2021-03-26 16:13 UTC (permalink / raw)
  To: Lukas Straub
  Cc: alexandr.iarygin, qemu-devel, Li Zhang, pankaj.gupta, Li Zhang,
	marcandre.lureau

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 :)



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-26 14:40     ` Markus Armbruster
  2021-03-26 15:19       ` Li Zhang
@ 2021-04-12 12:41       ` Li Zhang
  2021-04-12 13:10         ` Markus Armbruster
  1 sibling, 1 reply; 24+ messages in thread
From: Li Zhang @ 2021-04-12 12:41 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, alexandr.iarygin, QEMU, Li Zhang, Pankaj Gupta,
	Li Zhang, marcandre.lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-12 12:41       ` Li Zhang
@ 2021-04-12 13:10         ` Markus Armbruster
  2021-04-12 13:41           ` Li Zhang
  0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2021-04-12 13:10 UTC (permalink / raw)
  To: Li Zhang
  Cc: Lukas Straub, alexandr.iarygin, QEMU, Li Zhang, Pankaj Gupta,
	Li Zhang, marcandre.lureau

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"?



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-12 13:10         ` Markus Armbruster
@ 2021-04-12 13:41           ` Li Zhang
  0 siblings, 0 replies; 24+ messages in thread
From: Li Zhang @ 2021-04-12 13:41 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, alexandr.iarygin, QEMU, Li Zhang, Pankaj Gupta,
	Li Zhang, marcandre.lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
  2021-03-23 13:14   ` Li Zhang
@ 2021-04-12 14:19   ` Pankaj Gupta
  2021-04-13  6:40   ` Markus Armbruster
  2 siblings, 0 replies; 24+ messages in thread
From: Pankaj Gupta @ 2021-04-12 14:19 UTC (permalink / raw)
  To: Li Zhang
  Cc: lukasstraub2, alexandr.iarygin, armbru, Qemu Developers,
	Li Zhang, Pankaj Gupta, marcandre.lureau

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>


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
  2021-03-23 13:14   ` Li Zhang
  2021-04-12 14:19   ` Pankaj Gupta
@ 2021-04-13  6:40   ` Markus Armbruster
  2021-04-13  8:51     ` Li Zhang
  2021-04-13  8:57     ` Daniel P. Berrangé
  2 siblings, 2 replies; 24+ messages in thread
From: Markus Armbruster @ 2021-04-13  6:40 UTC (permalink / raw)
  To: Li Zhang
  Cc: lukasstraub2, alexandr.iarygin, qemu-devel, Li Zhang,
	pankaj.gupta, marcandre.lureau

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



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-13  6:40   ` Markus Armbruster
@ 2021-04-13  8:51     ` Li Zhang
  2021-04-16  9:33       ` Markus Armbruster
  2021-04-13  8:57     ` Daniel P. Berrangé
  1 sibling, 1 reply; 24+ messages in thread
From: Li Zhang @ 2021-04-13  8:51 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: lukasstraub2, alexandr.iarygin, qemu-devel, Li Zhang,
	pankaj.gupta, marcandre.lureau

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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-13  6:40   ` Markus Armbruster
  2021-04-13  8:51     ` Li Zhang
@ 2021-04-13  8:57     ` Daniel P. Berrangé
  2021-04-13  9:33       ` Li Zhang
  1 sibling, 1 reply; 24+ messages in thread
From: Daniel P. Berrangé @ 2021-04-13  8:57 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: lukasstraub2, alexandr.iarygin, qemu-devel, Li Zhang,
	pankaj.gupta, Li Zhang, marcandre.lureau

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 :|



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-13  8:57     ` Daniel P. Berrangé
@ 2021-04-13  9:33       ` Li Zhang
  0 siblings, 0 replies; 24+ messages in thread
From: Li Zhang @ 2021-04-13  9:33 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: lukasstraub2, alexandr.iarygin, Markus Armbruster, qemu-devel,
	Li Zhang, pankaj.gupta, marcandre.lureau

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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-13  8:51     ` Li Zhang
@ 2021-04-16  9:33       ` Markus Armbruster
  2021-04-16  9:52         ` Li Zhang
  0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2021-04-16  9:33 UTC (permalink / raw)
  To: Li Zhang
  Cc: lukasstraub2, alexandr.iarygin, qemu-devel, Li Zhang,
	pankaj.gupta, marcandre.lureau

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.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] Support monitor chardev hotswap with QMP
  2021-04-16  9:33       ` Markus Armbruster
@ 2021-04-16  9:52         ` Li Zhang
  0 siblings, 0 replies; 24+ messages in thread
From: Li Zhang @ 2021-04-16  9:52 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: lukasstraub2, alexandr.iarygin, qemu-devel, Li Zhang,
	pankaj.gupta, marcandre.lureau

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


^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-04-16  9:53 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 17:06 [PATCH 1/2] Fix the segment fault when calling yank_register_instance Li Zhang
2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
2021-03-23 13:14   ` Li Zhang
2021-03-26 14:40     ` Markus Armbruster
2021-03-26 15:19       ` Li Zhang
2021-04-12 12:41       ` Li Zhang
2021-04-12 13:10         ` Markus Armbruster
2021-04-12 13:41           ` Li Zhang
2021-04-12 14:19   ` Pankaj Gupta
2021-04-13  6:40   ` Markus Armbruster
2021-04-13  8:51     ` Li Zhang
2021-04-16  9:33       ` Markus Armbruster
2021-04-16  9:52         ` Li Zhang
2021-04-13  8:57     ` Daniel P. Berrangé
2021-04-13  9:33       ` Li Zhang
2021-03-15 18:51 ` [PATCH 1/2] Fix the segment fault when calling yank_register_instance Marc-André Lureau
2021-03-16  9:45   ` Li Zhang
2021-03-16 14:46   ` Li Zhang
2021-03-16 15:24     ` Marc-André Lureau
2021-03-16 15:36       ` Li Zhang
2021-03-17 21:06 ` Lukas Straub
2021-03-26 14:41 ` Markus Armbruster
2021-03-26 16:02   ` Lukas Straub
2021-03-26 16:13     ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).