* [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: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-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
* 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 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
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).