diff mbox series

[PATCHv2,1/1] Support monitor chardev hotswap with QMP

Message ID 20210413213459.629963-1-li.zhang@cloud.ionos.com (mailing list archive)
State New
Headers show
Series [PATCHv2,1/1] Support monitor chardev hotswap with QMP | expand

Commit Message

Li Zhang April 13, 2021, 9:34 p.m. UTC
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(-)

Comments

Li Zhang April 15, 2021, 3:07 p.m. UTC | #1
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
>
Markus Armbruster April 16, 2021, 2:50 p.m. UTC | #2
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);
>      }
>  }
Marc-André Lureau April 16, 2021, 2:59 p.m. UTC | #3
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);
> >      }
> >  }
>
>
>
Li Zhang April 16, 2021, 3:20 p.m. UTC | #4
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
Marc-André Lureau April 16, 2021, 3:28 p.m. UTC | #5
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
>
Li Zhang April 16, 2021, 3:46 p.m. UTC | #6
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
Markus Armbruster April 17, 2021, 8:02 a.m. UTC | #7
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.
Markus Armbruster April 17, 2021, 8:05 a.m. UTC | #8
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 :)

[...]
Li Zhang April 19, 2021, 11:56 a.m. UTC | #9
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.
Pankaj Gupta May 4, 2021, 6:29 a.m. UTC | #10
+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
Daniel P. Berrangé May 4, 2021, 8:38 a.m. UTC | #11
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
diff mbox series

Patch

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