qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 1/1] Support monitor chardev hotswap with QMP
@ 2021-04-13 21:34 Li Zhang
  2021-04-15 15:07 ` Li Zhang
  2021-04-16 14:50 ` Markus Armbruster
  0 siblings, 2 replies; 12+ messages in thread
From: Li Zhang @ 2021-04-13 21:34 UTC (permalink / raw)
  To: armbru, marcandre.lureau, qemu-devel; +Cc: Li Zhang, Pankaj Gupta

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>
Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
---
 v1 -> v2: 
  - Change mutex lock mon_lock section
  - Fix indentation problems

 monitor/monitor-internal.h |  3 +++
 monitor/monitor.c          |  2 +-
 monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..1b80c74883 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
 void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
 int hmp_compare_cmd(const char *name, const char *list);
 
+gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
+                           void *opaque);
+
 #endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
 
@@ -477,7 +478,36 @@ 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);
+    }
+
+    qemu_mutex_lock(&mon->common.mon_lock);
+    if (mon->common.out_watch) {
+        g_source_remove(mon->common.out_watch);
+        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;
@@ -487,7 +517,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);
 }
 
@@ -528,7 +565,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] 12+ messages in thread

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-13 21:34 [PATCHv2 1/1] Support monitor chardev hotswap with QMP Li Zhang
@ 2021-04-15 15:07 ` Li Zhang
  2021-04-16 14:50 ` Markus Armbruster
  1 sibling, 0 replies; 12+ messages in thread
From: Li Zhang @ 2021-04-15 15:07 UTC (permalink / raw)
  To: Markus Armbruster, marcandre.lureau, qemu-devel; +Cc: Li Zhang, Pankaj Gupta

Ping

On Tue, Apr 13, 2021 at 11:37 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
> 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>
> Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
> ---
>  v1 -> v2:
>   - Change mutex lock mon_lock section
>   - Fix indentation problems
>
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          |  2 +-
>  monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..1b80c74883 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
>  int hmp_compare_cmd(const char *name, const char *list);
>
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +                           void *opaque);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
>
> @@ -477,7 +478,36 @@ 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);
> +    }
> +
> +    qemu_mutex_lock(&mon->common.mon_lock);
> +    if (mon->common.out_watch) {
> +        g_source_remove(mon->common.out_watch);
> +        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;
> @@ -487,7 +517,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);
>  }
>
> @@ -528,7 +565,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
>


-- 

Best Regards
-Li


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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-13 21:34 [PATCHv2 1/1] Support monitor chardev hotswap with QMP Li Zhang
  2021-04-15 15:07 ` Li Zhang
@ 2021-04-16 14:50 ` Markus Armbruster
  2021-04-16 14:59   ` Marc-André Lureau
  2021-04-16 15:20   ` Li Zhang
  1 sibling, 2 replies; 12+ messages in thread
From: Markus Armbruster @ 2021-04-16 14:50 UTC (permalink / raw)
  To: Li Zhang; +Cc: marcandre.lureau, qemu-devel, Pankaj Gupta, Li Zhang

Marc-André, I'd like your opinion for this one, in particular the use of
g_source_remove().

Li Zhang <zhlcindy@gmail.com> writes:

> 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>
> Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>

Your commit message starts with a brief description of the problem.
Appreciated!  But I think it could be a bit clearer.  What about this:

    qmp: Support chardev-change

    For some scenarios, we'd like to hot-add a monitor device.  But QEMU
    doesn't support that, yet.  It does support hot-swapping character
    backends with QMP command chardev-change.  This lets us pre-add a
    monitor with a null character backend, then chardev-change to a
    socket backend.  Except the chardev-change fails with "Chardev user
    does not support chardev hotswap" because monitors don't provide the
    required callback.  Implement it for QMP monitors.

> ---
>  v1 -> v2: 
>   - Change mutex lock mon_lock section
>   - Fix indentation problems
>
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          |  2 +-
>  monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..1b80c74883 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
>  int hmp_compare_cmd(const char *name, const char *list);
>  
> +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +                           void *opaque);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
>  
> @@ -477,7 +478,36 @@ 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);
> +    }
> +
> +    qemu_mutex_lock(&mon->common.mon_lock);
> +    if (mon->common.out_watch) {
> +        g_source_remove(mon->common.out_watch);
> +        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
> +                                G_IO_OUT | G_IO_HUP,
> +                                monitor_unblocked,
> +                                &mon->common);

Visually align the arguments:

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

You may reduce argument indentation some to gain extra space, but keep
the arguments more indented than the function:

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

Do this only when you actually use the extra space for readability.

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

As I wrote in my review of v1, this function copies from
monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked().
Feels like a refactoring would be in order.  Doing it on top might be
easier.

> +
> +static void monitor_qmp_set_handlers_bh(void *opaque)
>  {
>      MonitorQMP *mon = opaque;
>      GMainContext *context;
> @@ -487,7 +517,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);
>  }
>  
> @@ -528,7 +565,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] 12+ messages in thread

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-16 14:50 ` Markus Armbruster
@ 2021-04-16 14:59   ` Marc-André Lureau
  2021-04-16 15:28     ` Marc-André Lureau
  2021-04-16 15:20   ` Li Zhang
  1 sibling, 1 reply; 12+ messages in thread
From: Marc-André Lureau @ 2021-04-16 14:59 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Li Zhang, Li Zhang, QEMU, Pankaj Gupta

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

Hi

On Fri, Apr 16, 2021 at 6:51 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André, I'd like your opinion for this one, in particular the use of
> g_source_remove().
>

My opinion isn't really worth much, my review would have a bit more value.

GSource has indeed some peculiar lifetime management, that I got wrong in
the past. So I would be extra careful.

But before spending time on review, I would also clarify the motivation and
ask for testing.

Markus, hot-adding/removing monitors isn't supported?


> Li Zhang <zhlcindy@gmail.com> writes:
>
> > 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>
> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
>
> Your commit message starts with a brief description of the problem.
> Appreciated!  But I think it could be a bit clearer.  What about this:
>
>     qmp: Support chardev-change
>
>     For some scenarios, we'd like to hot-add a monitor device.  But QEMU
>     doesn't support that, yet.  It does support hot-swapping character
>     backends with QMP command chardev-change.  This lets us pre-add a
>     monitor with a null character backend, then chardev-change to a
>     socket backend.  Except the chardev-change fails with "Chardev user
>     does not support chardev hotswap" because monitors don't provide the
>     required callback.  Implement it for QMP monitors.
>
> > ---
> >  v1 -> v2:
> >   - Change mutex lock mon_lock section
> >   - Fix indentation problems
> >
> >  monitor/monitor-internal.h |  3 +++
> >  monitor/monitor.c          |  2 +-
> >  monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
> >  3 files changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index 9c3a09cb01..1b80c74883 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
> >  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
> >  int hmp_compare_cmd(const char *name, const char *list);
> >
> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +                           void *opaque);
> > +
> >  #endif
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
> >
> > @@ -477,7 +478,36 @@ 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);
> > +    }
> > +
> > +    qemu_mutex_lock(&mon->common.mon_lock);
> > +    if (mon->common.out_watch) {
> > +        g_source_remove(mon->common.out_watch);
> > +        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
> > +                                G_IO_OUT | G_IO_HUP,
> > +                                monitor_unblocked,
> > +                                &mon->common);
>
> Visually align the arguments:
>
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                                       G_IO_OUT | G_IO_HUP,
>                                                       monitor_unblocked,
>                                                       &mon->common);
>
> You may reduce argument indentation some to gain extra space, but keep
> the arguments more indented than the function:
>
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                         G_IO_OUT | G_IO_HUP,
>                                         monitor_unblocked, &mon->common);
>
> Do this only when you actually use the extra space for readability.
>
> > +    }
> > +    qemu_mutex_unlock(&mon->common.mon_lock);
> > +
> > +    return 0;
> > +}
>
> As I wrote in my review of v1, this function copies from
> monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked().
> Feels like a refactoring would be in order.  Doing it on top might be
> easier.
>
> > +
> > +static void monitor_qmp_set_handlers_bh(void *opaque)
> >  {
> >      MonitorQMP *mon = opaque;
> >      GMainContext *context;
> > @@ -487,7 +517,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);
> >  }
> >
> > @@ -528,7 +565,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);
> >      }
> >  }
>
>
>

-- 
Marc-André Lureau

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

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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-16 14:50 ` Markus Armbruster
  2021-04-16 14:59   ` Marc-André Lureau
@ 2021-04-16 15:20   ` Li Zhang
  2021-04-17  8:05     ` Markus Armbruster
  1 sibling, 1 reply; 12+ messages in thread
From: Li Zhang @ 2021-04-16 15:20 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, Pankaj Gupta, Li Zhang

On Fri, Apr 16, 2021 at 4:50 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André, I'd like your opinion for this one, in particular the use of
> g_source_remove().
>
> Li Zhang <zhlcindy@gmail.com> writes:
>
> > 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>
> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
>
> Your commit message starts with a brief description of the problem.
> Appreciated!  But I think it could be a bit clearer.  What about this:
>
>     qmp: Support chardev-change
>
>     For some scenarios, we'd like to hot-add a monitor device.  But QEMU
>     doesn't support that, yet.  It does support hot-swapping character
>     backends with QMP command chardev-change.  This lets us pre-add a
>     monitor with a null character backend, then chardev-change to a
>     socket backend.  Except the chardev-change fails with "Chardev user
>     does not support chardev hotswap" because monitors don't provide the
>     required callback.  Implement it for QMP monitors.
>

It looks good to me.

> > ---
> >  v1 -> v2:
> >   - Change mutex lock mon_lock section
> >   - Fix indentation problems
> >
> >  monitor/monitor-internal.h |  3 +++
> >  monitor/monitor.c          |  2 +-
> >  monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
> >  3 files changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index 9c3a09cb01..1b80c74883 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
> >  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
> >  int hmp_compare_cmd(const char *name, const char *list);
> >
> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +                           void *opaque);
> > +
> >  #endif
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
> >
> > @@ -477,7 +478,36 @@ 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);
> > +    }
> > +
> > +    qemu_mutex_lock(&mon->common.mon_lock);
> > +    if (mon->common.out_watch) {
> > +        g_source_remove(mon->common.out_watch);
> > +        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
> > +                                G_IO_OUT | G_IO_HUP,
> > +                                monitor_unblocked,
> > +                                &mon->common);
>
> Visually align the arguments:
>
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                                       G_IO_OUT | G_IO_HUP,
>                                                       monitor_unblocked,
>                                                       &mon->common);
>

I double check my patch, it is like the above style. But it looks
different in email. It's really strange.
I will correct it.

> You may reduce argument indentation some to gain extra space, but keep
> the arguments more indented than the function:
>
>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>                                         G_IO_OUT | G_IO_HUP,
>                                         monitor_unblocked, &mon->common);
>
> Do this only when you actually use the extra space for readability.
>
OK, thanks for explanation.

> > +    }
> > +    qemu_mutex_unlock(&mon->common.mon_lock);
> > +
> > +    return 0;
> > +}
>
> As I wrote in my review of v1, this function copies from
> monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked().
> Feels like a refactoring would be in order.  Doing it on top might be
> easier.

 I may misunderstand what you mean. What's the meaning of "doing it on
top" exactly?

>
> > +
> > +static void monitor_qmp_set_handlers_bh(void *opaque)
> >  {
> >      MonitorQMP *mon = opaque;
> >      GMainContext *context;
> > @@ -487,7 +517,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);
> >  }
> >
> > @@ -528,7 +565,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] 12+ messages in thread

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-16 14:59   ` Marc-André Lureau
@ 2021-04-16 15:28     ` Marc-André Lureau
  2021-04-16 15:46       ` Li Zhang
  2021-04-17  8:02       ` Markus Armbruster
  0 siblings, 2 replies; 12+ messages in thread
From: Marc-André Lureau @ 2021-04-16 15:28 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Li Zhang, Li Zhang, QEMU, Pankaj Gupta

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

Hi

On Fri, Apr 16, 2021 at 6:59 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Fri, Apr 16, 2021 at 6:51 PM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Marc-André, I'd like your opinion for this one, in particular the use of
>> g_source_remove().
>>
>
> My opinion isn't really worth much, my review would have a bit more value.
>
> GSource has indeed some peculiar lifetime management, that I got wrong in
> the past. So I would be extra careful.
>
> But before spending time on review, I would also clarify the motivation
> and ask for testing.
>
> Markus, hot-adding/removing monitors isn't supported?
>
>
I realize you answered my question below. That's surprising me. Wouldn't it
make more sense to support it rather than having a pre-opened null-based
monitor that can have its chardev swapped?


>
>> Li Zhang <zhlcindy@gmail.com> writes:
>>
>> > 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>
>> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
>>
>> Your commit message starts with a brief description of the problem.
>> Appreciated!  But I think it could be a bit clearer.  What about this:
>>
>>     qmp: Support chardev-change
>>
>>     For some scenarios, we'd like to hot-add a monitor device.  But QEMU
>>     doesn't support that, yet.  It does support hot-swapping character
>>     backends with QMP command chardev-change.  This lets us pre-add a
>>     monitor with a null character backend, then chardev-change to a
>>     socket backend.  Except the chardev-change fails with "Chardev user
>>     does not support chardev hotswap" because monitors don't provide the
>>     required callback.  Implement it for QMP monitors.
>>
>> > ---
>> >  v1 -> v2:
>> >   - Change mutex lock mon_lock section
>> >   - Fix indentation problems
>> >
>> >  monitor/monitor-internal.h |  3 +++
>> >  monitor/monitor.c          |  2 +-
>> >  monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
>> >  3 files changed, 44 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>> > index 9c3a09cb01..1b80c74883 100644
>> > --- a/monitor/monitor-internal.h
>> > +++ b/monitor/monitor-internal.h
>> > @@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
>> >  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
>> >  int hmp_compare_cmd(const char *name, const char *list);
>> >
>> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>> > +                           void *opaque);
>> > +
>> >  #endif
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
>> >
>> > @@ -477,7 +478,36 @@ 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);
>> > +    }
>> > +
>> > +    qemu_mutex_lock(&mon->common.mon_lock);
>> > +    if (mon->common.out_watch) {
>> > +        g_source_remove(mon->common.out_watch);
>> > +        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>> > +                                G_IO_OUT | G_IO_HUP,
>> > +                                monitor_unblocked,
>> > +                                &mon->common);
>>
>> Visually align the arguments:
>>
>>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>>                                                       G_IO_OUT | G_IO_HUP,
>>                                                       monitor_unblocked,
>>                                                       &mon->common);
>>
>> You may reduce argument indentation some to gain extra space, but keep
>> the arguments more indented than the function:
>>
>>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>>                                         G_IO_OUT | G_IO_HUP,
>>                                         monitor_unblocked, &mon->common);
>>
>> Do this only when you actually use the extra space for readability.
>>
>> > +    }
>> > +    qemu_mutex_unlock(&mon->common.mon_lock);
>> > +
>> > +    return 0;
>> > +}
>>
>> As I wrote in my review of v1, this function copies from
>> monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked().
>> Feels like a refactoring would be in order.  Doing it on top might be
>> easier.
>>
>> > +
>> > +static void monitor_qmp_set_handlers_bh(void *opaque)
>> >  {
>> >      MonitorQMP *mon = opaque;
>> >      GMainContext *context;
>> > @@ -487,7 +517,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);
>> >  }
>> >
>> > @@ -528,7 +565,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);
>> >      }
>> >  }
>>
>>
>>
>
> --
> Marc-André Lureau
>


-- 
Marc-André Lureau

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

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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-16 15:28     ` Marc-André Lureau
@ 2021-04-16 15:46       ` Li Zhang
  2021-04-17  8:02       ` Markus Armbruster
  1 sibling, 0 replies; 12+ messages in thread
From: Li Zhang @ 2021-04-16 15:46 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Li Zhang, Markus Armbruster, Pankaj Gupta, QEMU

Hi Marc-André,

On Fri, Apr 16, 2021 at 5:28 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Fri, Apr 16, 2021 at 6:59 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>>
>> On Fri, Apr 16, 2021 at 6:51 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Marc-André, I'd like your opinion for this one, in particular the use of
>>> g_source_remove().
>>
>>
>> My opinion isn't really worth much, my review would have a bit more value.
>>
>> GSource has indeed some peculiar lifetime management, that I got wrong in the past. So I would be extra careful.
>>
>> But before spending time on review, I would also clarify the motivation and ask for testing.
>>
>> Markus, hot-adding/removing monitors isn't supported?
>>
>
> I realize you answered my question below. That's surprising me. Wouldn't it make more sense to support it rather than having a pre-opened null-based monitor that can have its chardev swapped?

It's the best way to support hot-add/remove monitor devices. But we
would like to use it as soon as possible, so I use hotswap currently.
If possible, we can consider implementing hot-add/remove in the future.


>
>>>
>>>
>>> Li Zhang <zhlcindy@gmail.com> writes:
>>>
>>> > 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>
>>> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
>>>
>>> Your commit message starts with a brief description of the problem.
>>> Appreciated!  But I think it could be a bit clearer.  What about this:
>>>
>>>     qmp: Support chardev-change
>>>
>>>     For some scenarios, we'd like to hot-add a monitor device.  But QEMU
>>>     doesn't support that, yet.  It does support hot-swapping character
>>>     backends with QMP command chardev-change.  This lets us pre-add a
>>>     monitor with a null character backend, then chardev-change to a
>>>     socket backend.  Except the chardev-change fails with "Chardev user
>>>     does not support chardev hotswap" because monitors don't provide the
>>>     required callback.  Implement it for QMP monitors.
>>>
>>> > ---
>>> >  v1 -> v2:
>>> >   - Change mutex lock mon_lock section
>>> >   - Fix indentation problems
>>> >
>>> >  monitor/monitor-internal.h |  3 +++
>>> >  monitor/monitor.c          |  2 +-
>>> >  monitor/qmp.c              | 43 +++++++++++++++++++++++++++++++++++---
>>> >  3 files changed, 44 insertions(+), 4 deletions(-)
>>> >
>>> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>>> > index 9c3a09cb01..1b80c74883 100644
>>> > --- a/monitor/monitor-internal.h
>>> > +++ b/monitor/monitor-internal.h
>>> > @@ -183,4 +183,7 @@ void help_cmd(Monitor *mon, const char *name);
>>> >  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
>>> >  int hmp_compare_cmd(const char *name, const char *list);
>>> >
>>> > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>>> > +                           void *opaque);
>>> > +
>>> >  #endif
>>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>>> > index 636bcc81c5..16a3620d02 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 2b0308f933..5fa65401ae 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;
>>> >
>>> > @@ -477,7 +478,36 @@ 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);
>>> > +    }
>>> > +
>>> > +    qemu_mutex_lock(&mon->common.mon_lock);
>>> > +    if (mon->common.out_watch) {
>>> > +        g_source_remove(mon->common.out_watch);
>>> > +        mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>>> > +                                G_IO_OUT | G_IO_HUP,
>>> > +                                monitor_unblocked,
>>> > +                                &mon->common);
>>>
>>> Visually align the arguments:
>>>
>>>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>>>                                                       G_IO_OUT | G_IO_HUP,
>>>                                                       monitor_unblocked,
>>>                                                       &mon->common);
>>>
>>> You may reduce argument indentation some to gain extra space, but keep
>>> the arguments more indented than the function:
>>>
>>>         mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr,
>>>                                         G_IO_OUT | G_IO_HUP,
>>>                                         monitor_unblocked, &mon->common);
>>>
>>> Do this only when you actually use the extra space for readability.
>>>
>>> > +    }
>>> > +    qemu_mutex_unlock(&mon->common.mon_lock);
>>> > +
>>> > +    return 0;
>>> > +}
>>>
>>> As I wrote in my review of v1, this function copies from
>>> monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked().
>>> Feels like a refactoring would be in order.  Doing it on top might be
>>> easier.
>>>
>>> > +
>>> > +static void monitor_qmp_set_handlers_bh(void *opaque)
>>> >  {
>>> >      MonitorQMP *mon = opaque;
>>> >      GMainContext *context;
>>> > @@ -487,7 +517,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);
>>> >  }
>>> >
>>> > @@ -528,7 +565,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);
>>> >      }
>>> >  }
>>>
>>>
>>
>>
>> --
>> Marc-André Lureau
>
>
>
> --
> Marc-André Lureau



-- 

Best Regards
-Li


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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-16 15:28     ` Marc-André Lureau
  2021-04-16 15:46       ` Li Zhang
@ 2021-04-17  8:02       ` Markus Armbruster
  2021-04-19 11:56         ` Li Zhang
  2021-05-04  6:29         ` Pankaj Gupta
  1 sibling, 2 replies; 12+ messages in thread
From: Markus Armbruster @ 2021-04-17  8:02 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Li Zhang, Li Zhang, QEMU, Pankaj Gupta

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Apr 16, 2021 at 6:59 PM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Fri, Apr 16, 2021 at 6:51 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>>
>>> Marc-André, I'd like your opinion for this one, in particular the use of
>>> g_source_remove().
>>>
>>
>> My opinion isn't really worth much, my review would have a bit more value.
>>
>> GSource has indeed some peculiar lifetime management, that I got wrong in
>> the past. So I would be extra careful.
>>
>> But before spending time on review, I would also clarify the motivation
>> and ask for testing.
>>
>> Markus, hot-adding/removing monitors isn't supported?
>>
>>
> I realize you answered my question below. That's surprising me. Wouldn't it
> make more sense to support it rather than having a pre-opened null-based
> monitor that can have its chardev swapped?

Yes, it would.  Patches welcome.

This patch is a somewhat ham-fisted and limited solution to the problem
stated in the commit message.  However, it might *also* be a reasonable
improvement to chardev-change on its own.  Not for me to judge.

chardev-change comes with a number of restrictions.  Let's have a closer
look.  It fails

1. when no such character device exists (d'oh)

2. for chardev-mux devices

3. in record/replay mode

4. when a backend is connected that doesn't implement the chr_be_change()
   method

5. when chr_be_change() fails

6. when creating the new chardev fails[*]

Items 2, 3, 4 are restrictions.  I figure 2 and 4 are simply not
implemented, yet.  I'm not sure about 3.

Whether we want to accept patches lifting restrictions is up to the
chardev maintainers.

This patch lifts restriction 4 for QMP monitor backends.  Its monitor
part looks acceptable to me, but I dislike its code duplication.  Before
we spend time on cleaning that up (or on deciding to clean it up later),
I'd like to hear the chardev mantainers' judgement, because that's about
more serious matters than cleanliness.

Do I make sense?

[...]


[*] The code for creating the new chardev in the "no backend connected"
case

    be = chr->be;
    if (!be) {
        /* easy case */
        object_unparent(OBJECT(chr));
        return qmp_chardev_add(id, backend, errp);
    }

is problematic: when qmp_chardev_add() fails, we already destroyed the
old chardev.  It should destroy the old chardev only when it can create
its replacement.



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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-16 15:20   ` Li Zhang
@ 2021-04-17  8:05     ` Markus Armbruster
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2021-04-17  8:05 UTC (permalink / raw)
  To: Li Zhang; +Cc: marcandre.lureau, Li Zhang, qemu-devel, Pankaj Gupta

Li Zhang <zhlcindy@gmail.com> writes:

> On Fri, Apr 16, 2021 at 4:50 PM Markus Armbruster <armbru@redhat.com> wrote:

[...]

>> As I wrote in my review of v1, this function copies from
>> monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked().
>> Feels like a refactoring would be in order.  Doing it on top might be
>> easier.
>
>  I may misunderstand what you mean. What's the meaning of "doing it on
> top" exactly?

As a separate patch on top of this one.  Sorry for the jargon :)

[...]



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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-17  8:02       ` Markus Armbruster
@ 2021-04-19 11:56         ` Li Zhang
  2021-05-04  6:29         ` Pankaj Gupta
  1 sibling, 0 replies; 12+ messages in thread
From: Li Zhang @ 2021-04-19 11:56 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Li Zhang, Marc-André Lureau, QEMU, Pankaj Gupta

On Sat, Apr 17, 2021 at 10:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> >
> > On Fri, Apr 16, 2021 at 6:59 PM Marc-André Lureau <
> > marcandre.lureau@gmail.com> wrote:
> >
> >> Hi
> >>
> >> On Fri, Apr 16, 2021 at 6:51 PM Markus Armbruster <armbru@redhat.com>
> >> wrote:
> >>
> >>> Marc-André, I'd like your opinion for this one, in particular the use of
> >>> g_source_remove().
> >>>
> >>
> >> My opinion isn't really worth much, my review would have a bit more value.
> >>
> >> GSource has indeed some peculiar lifetime management, that I got wrong in
> >> the past. So I would be extra careful.
> >>
> >> But before spending time on review, I would also clarify the motivation
> >> and ask for testing.
> >>
> >> Markus, hot-adding/removing monitors isn't supported?
> >>
> >>
> > I realize you answered my question below. That's surprising me. Wouldn't it
> > make more sense to support it rather than having a pre-opened null-based
> > monitor that can have its chardev swapped?
>
> Yes, it would.  Patches welcome.
>
> This patch is a somewhat ham-fisted and limited solution to the problem
> stated in the commit message.  However, it might *also* be a reasonable
> improvement to chardev-change on its own.  Not for me to judge.
>
Okay, Thanks.

> chardev-change comes with a number of restrictions.  Let's have a closer
> look.  It fails
>
> 1. when no such character device exists (d'oh)
>
> 2. for chardev-mux devices
>
> 3. in record/replay mode
>
> 4. when a backend is connected that doesn't implement the chr_be_change()
>    method
>
> 5. when chr_be_change() fails
>
> 6. when creating the new chardev fails[*]
>
> Items 2, 3, 4 are restrictions.  I figure 2 and 4 are simply not
> implemented, yet.  I'm not sure about 3.
>
For item 3,
source code mentions "chardev cannot  be changed in record/replay mode".
I never tried it yet. I am not quite sure why it couldn't be changed.

> Whether we want to accept patches lifting restrictions is up to the
> chardev maintainers.
>
> This patch lifts restriction 4 for QMP monitor backends.  Its monitor
> part looks acceptable to me, but I dislike its code duplication.  Before
> we spend time on cleaning that up (or on deciding to clean it up later),
> I'd like to hear the chardev mantainers' judgement, because that's about
> more serious matters than cleanliness.
>
> Do I make sense?
Yes, make sense.  Thanks.

>
> [...]
>
>
> [*] The code for creating the new chardev in the "no backend connected"
> case
>
>     be = chr->be;
>     if (!be) {
>         /* easy case */
>         object_unparent(OBJECT(chr));
>         return qmp_chardev_add(id, backend, errp);
>     }
>
> is problematic: when qmp_chardev_add() fails, we already destroyed the
> old chardev.  It should destroy the old chardev only when it can create
> its replacement.
>
You are right. It is a problem.


-- 

Best Regards
-Li


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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-04-17  8:02       ` Markus Armbruster
  2021-04-19 11:56         ` Li Zhang
@ 2021-05-04  6:29         ` Pankaj Gupta
  2021-05-04  8:38           ` Daniel P. Berrangé
  1 sibling, 1 reply; 12+ messages in thread
From: Pankaj Gupta @ 2021-05-04  6:29 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrange, QEMU, Pankaj Gupta, Li Zhang,
	Marc-André Lureau, Li Zhang

+CC Danpb

> >>> Marc-André, I'd like your opinion for this one, in particular the use of
> >>> g_source_remove().
> >>>
> >>
> >> My opinion isn't really worth much, my review would have a bit more value.
> >>
> >> GSource has indeed some peculiar lifetime management, that I got wrong in
> >> the past. So I would be extra careful.
> >>
> >> But before spending time on review, I would also clarify the motivation
> >> and ask for testing.
> >>
> >> Markus, hot-adding/removing monitors isn't supported?
> >>
> >>
> > I realize you answered my question below. That's surprising me. Wouldn't it
> > make more sense to support it rather than having a pre-opened null-based
> > monitor that can have its chardev swapped?
>
> Yes, it would.  Patches welcome.
>
> This patch is a somewhat ham-fisted and limited solution to the problem
> stated in the commit message.  However, it might *also* be a reasonable
> improvement to chardev-change on its own.  Not for me to judge.
>
> chardev-change comes with a number of restrictions.  Let's have a closer
> look.  It fails
>
> 1. when no such character device exists (d'oh)
>
> 2. for chardev-mux devices
>
> 3. in record/replay mode
>
> 4. when a backend is connected that doesn't implement the chr_be_change()
>    method
>
> 5. when chr_be_change() fails
>
> 6. when creating the new chardev fails[*]
>
> Items 2, 3, 4 are restrictions.  I figure 2 and 4 are simply not
> implemented, yet.  I'm not sure about 3.
>
> Whether we want to accept patches lifting restrictions is up to the
> chardev maintainers.

Maybe we can handle or already handle the restrictions at libvirt side?

>
> This patch lifts restriction 4 for QMP monitor backends.  Its monitor
> part looks acceptable to me, but I dislike its code duplication.  Before
> we spend time on cleaning that up (or on deciding to clean it up later),
> I'd like to hear the chardev mantainers' judgement, because that's about
> more serious matters than cleanliness.

Sure. But I also feel allowing to change monitor device is a useful feature
independent of monitor hotplug/unplug feature .

>
> Do I make sense?
>
> [...]
>
>
> [*] The code for creating the new chardev in the "no backend connected"
> case
>
>     be = chr->be;
>     if (!be) {
>         /* easy case */
>         object_unparent(OBJECT(chr));
>         return qmp_chardev_add(id, backend, errp);
>     }
>
> is problematic: when qmp_chardev_add() fails, we already destroyed the
> old chardev.  It should destroy the old chardev only when it can create
> its replacement.

 Good point. I agree. We should fix this.

Thanks,
Pankaj


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

* Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP
  2021-05-04  6:29         ` Pankaj Gupta
@ 2021-05-04  8:38           ` Daniel P. Berrangé
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-05-04  8:38 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: QEMU, Markus Armbruster, Li Zhang, Marc-André Lureau,
	Pankaj Gupta, Li Zhang

On Tue, May 04, 2021 at 08:29:50AM +0200, Pankaj Gupta wrote:
> +CC Danpb
> 
> > >>> Marc-André, I'd like your opinion for this one, in particular the use of
> > >>> g_source_remove().
> > >>>
> > >>
> > >> My opinion isn't really worth much, my review would have a bit more value.
> > >>
> > >> GSource has indeed some peculiar lifetime management, that I got wrong in
> > >> the past. So I would be extra careful.
> > >>
> > >> But before spending time on review, I would also clarify the motivation
> > >> and ask for testing.
> > >>
> > >> Markus, hot-adding/removing monitors isn't supported?
> > >>
> > >>
> > > I realize you answered my question below. That's surprising me. Wouldn't it
> > > make more sense to support it rather than having a pre-opened null-based
> > > monitor that can have its chardev swapped?
> >
> > Yes, it would.  Patches welcome.
> >
> > This patch is a somewhat ham-fisted and limited solution to the problem
> > stated in the commit message.  However, it might *also* be a reasonable
> > improvement to chardev-change on its own.  Not for me to judge.
> >
> > chardev-change comes with a number of restrictions.  Let's have a closer
> > look.  It fails
> >
> > 1. when no such character device exists (d'oh)
> >
> > 2. for chardev-mux devices
> >
> > 3. in record/replay mode
> >
> > 4. when a backend is connected that doesn't implement the chr_be_change()
> >    method
> >
> > 5. when chr_be_change() fails
> >
> > 6. when creating the new chardev fails[*]
> >
> > Items 2, 3, 4 are restrictions.  I figure 2 and 4 are simply not
> > implemented, yet.  I'm not sure about 3.
> >
> > Whether we want to accept patches lifting restrictions is up to the
> > chardev maintainers.
> 
> Maybe we can handle or already handle the restrictions at libvirt side?

From the libvirt side I'd just like to see the real problem addressed.
ie actally implement monitor hotplug/unplug, instead of this hack
where you pre-create a monitor with null backend.


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] 12+ messages in thread

end of thread, other threads:[~2021-05-04  8:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 21:34 [PATCHv2 1/1] Support monitor chardev hotswap with QMP Li Zhang
2021-04-15 15:07 ` Li Zhang
2021-04-16 14:50 ` Markus Armbruster
2021-04-16 14:59   ` Marc-André Lureau
2021-04-16 15:28     ` Marc-André Lureau
2021-04-16 15:46       ` Li Zhang
2021-04-17  8:02       ` Markus Armbruster
2021-04-19 11:56         ` Li Zhang
2021-05-04  6:29         ` Pankaj Gupta
2021-05-04  8:38           ` Daniel P. Berrangé
2021-04-16 15:20   ` Li Zhang
2021-04-17  8:05     ` 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).