在 2021/6/8 23:37, Daniel P. Berrangé 写道:
> On Tue, Jun 08, 2021 at 04:07:30PM +0200, Markus Armbruster wrote:
>> "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
>> <longpeng2@huawei.com> writes:
>>
>>> We find a race during QEMU starting, which would case the QEMU process coredump.
>>>
>>> <main loop>                             |    <MON iothread>
>>>                                         |
>>> [1] create MON chardev                  |
>>> qemu_create_early_backends              |
>>>   chardev_init_func                     |
>>>                                         |
>>> [2] create MON iothread                 |
>>> qemu_create_late_backends               |
>>>   mon_init_func                         |
>>>     aio_bh_schedule-----------------------> monitor_qmp_setup_handlers_bh
>>> [3] enter main loog                     |    tcp_chr_update_read_handler
>>> (* A client come in, e.g. Libvirt *)    |      update_ioc_handlers
>>> tcp_chr_new_client                      |
>>>   update_ioc_handlers                   |
>>>                                         |
>>>     [4] create new hup_source           |
>>>         s->hup_source = *PTR1*          |
>>>           g_source_attach(s->hup_source)|
>>>                                         |        [5] remove_hup_source(*PTR1*)
>>>                                         |            (create new hup_source)
>>>                                         |             s->hup_source = *PTR2*
>>>         [6] g_source_attach_unlocked    |
>>>               *PTR1* is freed by [5]    |
>>>                     
>>> Do you have any suggestion to fix this bug ? Thanks!
>>
>> Do we?  We talked, but I'm not sure we reached a conclusion.
>
> Seems like we ended up with two options.
>
>   1. A workaround for the current  specific problem by rearranging
>      the initilization code in the monitor a little.
>
>   2. A design fix of splitting the chardev creation into two
>      parts, one creation, and one activation.
>
> The latter is significantly more work, but is a better long term bet IMHO.
> But what we really is someone motivated to actually implement one of the
> two options.
>

How about the following implementation of option-1 ? We've tested it for several
weeks, it works fine.

diff --git a/chardev/char-socket.c b/chardev/char-socket.c

index a484641..ecb3db9 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -722,6 +722,19 @@ static void tcp_chr_update_read_handler(Chardev *chr)
     update_ioc_handlers(s);
 }

+static void tcp_chr_disable_handler(Chardev *chr)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (s->listener && s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
+        qio_net_listener_set_client_func_full(s->listener, NULL, NULL,
+                                              NULL, chr->gcontext);
+    }
+
+    remove_fd_in_watch(chr);
+    remove_hup_source(s);
+}
+
 static bool tcp_chr_is_connected(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -1703,6 +1716,7 @@ static void char_socket_class_init(ObjectClass *oc, void
*data)
     cc->chr_add_watch = tcp_chr_add_watch;
     cc->chr_set_reconnect_time = tcp_chr_set_reconnect_time;
     cc->chr_update_read_handler = tcp_chr_update_read_handler;
+    cc->chr_disable_handler = tcp_chr_disable_handler;
     cc->chr_is_connected = tcp_chr_is_connected;
     cc->chr_get_connect_id = tcp_chr_get_connect_id;

diff --git a/chardev/char.c b/chardev/char.c
index ff0a3cf..990fe4f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -238,6 +238,15 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
     }
 }

+void qemu_chr_be_disable_handlers(Chardev *s)
+{
+    ChardevClass *cc = CHARDEV_GET_CLASS(s);
+
+    if (cc->chr_disable_handler) {
+        cc->chr_disable_handler(s);
+    }
+}
+
 int qemu_chr_add_client(Chardev *s, int fd)
 {
     return CHARDEV_GET_CLASS(s)->chr_add_client ?
diff --git a/include/chardev/char.h b/include/chardev/char.h
index d1ec628..7a8c740 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -212,6 +212,8 @@ void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len);
 void qemu_chr_be_update_read_handlers(Chardev *s,
                                       GMainContext *context);

+void qemu_chr_be_disable_handlers(Chardev *s);
+
 /**
  * qemu_chr_be_event:
  * @event: the event to send
@@ -282,6 +284,7 @@ typedef struct ChardevClass {
     int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
     GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
     void (*chr_update_read_handler)(Chardev *s);
+    void (*chr_disable_handler)(Chardev *s);
     int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
     int (*get_msgfds)(Chardev *s, int* fds, int num);
     int (*set_msgfds)(Chardev *s, int *fds, int num);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 9a69ae4..2c2248c 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -413,11 +413,13 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
          * e.g. the chardev is in client mode, with wait=on.
          */
         remove_fd_in_watch(chr);
+
         /*
-         * We can't call qemu_chr_fe_set_handlers() directly here
-         * since chardev might be running in the monitor I/O
-         * thread.  Schedule a bottom half.
+         * Before schedule a bottom half, we should clean up the handler in the
+         * default context to prevent the race between main thread and iothread
          */
+        qemu_chr_be_disable_handlers(chr);
+
         aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
                                 monitor_qmp_setup_handlers_bh, mon);
         /* The bottom half will add @mon to @mon_list */
--
1.8.3.1


> Regards,
> Daniel
>