This is needed to keep sending DEVICE_DELETED events on qemu cleanup.
The event may happen in the rcu thread and we're going to flush the rcu queue
explicitly before qemu exiting in the next patch. So move the monitor
destruction to the very end of qemu cleanup to be able to send all the events.

Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
---
 monitor/monitor.c  | 6 ++++++
 softmmu/runstate.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 21c7a68758f5..b04ae4850db2 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -605,11 +605,17 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
     mon->outbuf = g_string_new(NULL);
     mon->skip_flush = skip_flush;
     mon->use_io_thread = use_io_thread;
+    /*
+     * take an extra ref to prevent monitor's chardev
+     * from destroying in qemu_chr_cleanup()
+     */
+    object_ref(OBJECT(mon->chr.chr));
 }
 
 void monitor_data_destroy(Monitor *mon)
 {
     g_free(mon->mon_cpu_path);
+    object_unref(OBJECT(mon->chr.chr));
     qemu_chr_fe_deinit(&mon->chr, false);
     if (monitor_is_qmp(mon)) {
         monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 10d9b7365aa7..8d29dd2c00e2 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -819,8 +819,8 @@ void qemu_cleanup(void)
     tpm_cleanup();
     net_cleanup();
     audio_cleanup();
-    monitor_cleanup();
     qemu_chr_cleanup();
     user_creatable_cleanup();
+    monitor_cleanup();
     /* TODO: unref root container, check all devices are ok */
 }
-- 
2.25.1

Denis Plotnikov <den-plotnikov@yandex-team.ru> writes:

> This is needed to keep sending DEVICE_DELETED events on qemu cleanup.
> The event may happen in the rcu thread and we're going to flush the rcu queue
> explicitly before qemu exiting in the next patch. So move the monitor
> destruction to the very end of qemu cleanup to be able to send all the events.
>
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
>  monitor/monitor.c  | 6 ++++++
>  softmmu/runstate.c | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 21c7a68758f5..b04ae4850db2 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -605,11 +605,17 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>      mon->outbuf = g_string_new(NULL);
>      mon->skip_flush = skip_flush;
>      mon->use_io_thread = use_io_thread;
> +    /*
> +     * take an extra ref to prevent monitor's chardev
> +     * from destroying in qemu_chr_cleanup()
> +     */
> +    object_ref(OBJECT(mon->chr.chr));

I'm not sure we need the comment in the long run.

Taking a reference changes mon->chr.chr from soft reference to hard
reference.  Feels right to me.

Note that mon->chr.chr isn't set here, but earlier.  Unlike the other
parts of @mon.  Because of this it starts as a soft reference, and
hardens only here.

It's set in three places:

1. monitor_init_hmp():

    if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
        g_free(mon);
        return;
    }

    monitor_data_init(&mon->common, false, false, false);

2. monitor_init_qmp():

    if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
        g_free(mon);
        return;
    }
    qemu_chr_fe_set_echo(&mon->common.chr, true);

    /* Note: we run QMP monitor in I/O thread when @chr supports that */
    monitor_data_init(&mon->common, true, false,
                      qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));

3. qmp_human_monitor_command()

    MonitorHMP hmp = {};

    monitor_data_init(&hmp.common, false, true, false);

   hmp.common.chr.chr remains null here.  Works, because
   object_ref(OBJECT(NULL)) and object_unref(OBJECT(NULL)) do nothing.

Slightly cleaner, I think: pass the character device as an argument to
monitor_data_init(), take a reference and store it in mon->chr.chr
there.

>  }
>  
>  void monitor_data_destroy(Monitor *mon)
>  {
>      g_free(mon->mon_cpu_path);
> +    object_unref(OBJECT(mon->chr.chr));
>      qemu_chr_fe_deinit(&mon->chr, false);
>      if (monitor_is_qmp(mon)) {
>          monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index 10d9b7365aa7..8d29dd2c00e2 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -819,8 +819,8 @@ void qemu_cleanup(void)
>      tpm_cleanup();
>      net_cleanup();
>      audio_cleanup();
> -    monitor_cleanup();
>      qemu_chr_cleanup();
>      user_creatable_cleanup();
> +    monitor_cleanup();
>      /* TODO: unref root container, check all devices are ok */
>  }

Monitor cleanup now runs with character devices that have been
"unparented" from the QOM tree.  Paolo, is that okay?