| Message ID | 20210315170636.704201-2-zhlcindy@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] Fix the segment fault when calling yank_register_instance | expand |
Hi, Any comments about this patch? Thanks Li On Mon, Mar 15, 2021 at 6:07 PM Li Zhang <zhlcindy@gmail.com> wrote: > From: Li Zhang <li.zhang@cloud.ionos.com> > > 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> > --- > monitor/monitor-internal.h | 3 +++ > monitor/monitor.c | 2 +- > monitor/qmp.c | 42 +++++++++++++++++++++++++++++++++++--- > 3 files changed, 43 insertions(+), 4 deletions(-) > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > index 40903d6386..2df6dd21de 100644 > --- a/monitor/monitor-internal.h > +++ b/monitor/monitor-internal.h > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char > *list); > void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, > Error **errp); > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > + void *opaque); > + > #endif > diff --git a/monitor/monitor.c b/monitor/monitor.c > index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; > > @@ -480,7 +481,35 @@ 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); > + } > + > + if (mon->common.out_watch) { > + g_source_remove(mon->common.out_watch); > + qemu_mutex_lock(&mon->common.mon_lock); > + 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; > @@ -490,7 +519,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); > } > > @@ -531,7 +567,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 > >
Li Zhang <li.zhang@ionos.com> writes: > Hi, > > Any comments about this patch? I wanted to review this before my Easter break, but I'm out of time :( When I'm back (April 6), I'll check whether it still needs review, but I do hope somebody else can look at it sooner.
On Fri, Mar 26, 2021 at 3:40 PM Markus Armbruster <armbru@redhat.com> wrote: > Li Zhang <li.zhang@ionos.com> writes: > > > Hi, > > > > Any comments about this patch? > > I wanted to review this before my Easter break, but I'm out of time :( > > When I'm back (April 6), I'll check whether it still needs review, but I > do hope somebody else can look at it sooner. > > Thanks anyway. This patch is better to work with Lukas' patches together.
Hi Markus, Any suggestions on this patch? Thanks Li On Fri, Mar 26, 2021 at 3:40 PM Markus Armbruster <armbru@redhat.com> wrote: > Li Zhang <li.zhang@ionos.com> writes: > > > Hi, > > > > Any comments about this patch? > > I wanted to review this before my Easter break, but I'm out of time :( > > When I'm back (April 6), I'll check whether it still needs review, but I > do hope somebody else can look at it sooner. > >
Li Zhang <li.zhang@ionos.com> writes: > Hi Markus, > > Any suggestions on this patch? I understand PATCH 1/2 got superseded by Lukas's "[PATCH v8 0/4] yank: Add chardev tests and fixes". I trust Marc-André will take care of it in due time. Before I look at the actual patch: does this patch depend on Lukas's fix or your "[PATCH 1/2] Fix the segment fault when calling yank_register_instance"?
Hi Markus, [PATCH 1/2] Fix the segment fault when calling yank_register_instance is reworked by Lukas. And his patches have been merged to master branch of qemu. [PATCH 2/2] Support monitor chardev hotswap with QMP is to change monitor backend with chardev-change. It is not implemented yet. On Mon, Apr 12, 2021 at 3:10 PM Markus Armbruster <armbru@redhat.com> wrote: > Li Zhang <li.zhang@ionos.com> writes: > > > Hi Markus, > > > > Any suggestions on this patch? > > I understand PATCH 1/2 got superseded by Lukas's "[PATCH v8 0/4] yank: > Add chardev tests and fixes". I trust Marc-André will take care of it > in due time. > > Before I look at the actual patch: does this patch depend on Lukas's fix > or your "[PATCH 1/2] Fix the segment fault when calling > yank_register_instance"? > >
Hi Li, > 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> > --- > monitor/monitor-internal.h | 3 +++ > monitor/monitor.c | 2 +- > monitor/qmp.c | 42 +++++++++++++++++++++++++++++++++++--- > 3 files changed, 43 insertions(+), 4 deletions(-) > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > index 40903d6386..2df6dd21de 100644 > --- a/monitor/monitor-internal.h > +++ b/monitor/monitor-internal.h > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list); > void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, > Error **errp); > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > + void *opaque); > + > #endif > diff --git a/monitor/monitor.c b/monitor/monitor.c > index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; > > @@ -480,7 +481,35 @@ 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); > + } > + > + if (mon->common.out_watch) { > + g_source_remove(mon->common.out_watch); > + qemu_mutex_lock(&mon->common.mon_lock); Not very sure if mutex is must here, but still its protecting "out_watch" for simultaneous updating. > + 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; > @@ -490,7 +519,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); > } > > @@ -531,7 +567,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); > } > } Overall patch looks good to me. Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
Li Zhang <zhlcindy@gmail.com> writes: > From: Li Zhang <li.zhang@cloud.ionos.com> > > 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> I think what what you're trying to say is that chardev-change does not work when the character device changes is used by a QMP monitor. Correct? If yes, how exactly does it misbehave? Does it work with an HMP monitor? > --- > monitor/monitor-internal.h | 3 +++ > monitor/monitor.c | 2 +- > monitor/qmp.c | 42 +++++++++++++++++++++++++++++++++++--- > 3 files changed, 43 insertions(+), 4 deletions(-) > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > index 40903d6386..2df6dd21de 100644 > --- a/monitor/monitor-internal.h > +++ b/monitor/monitor-internal.h > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list); > void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, > Error **errp); > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > + void *opaque); > + > #endif > diff --git a/monitor/monitor.c b/monitor/monitor.c > index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; > > @@ -480,7 +481,35 @@ 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); > + } > + > + if (mon->common.out_watch) { > + g_source_remove(mon->common.out_watch); All other updates of @out_watch are under @mon_lock. Why not this one? I have no idea whether g_source_remove() is the right function to call. Its documentation says "You must use g_source_destroy() for sources added to a non-default main context." The qemu_chr_fe_set_handlers() contract is of no help. Documentation of g_source_destroy() confuses some more: "This does not unref the GSource: if you still hold a reference, use g_source_unref() to drop it. Marc-André, can you help? > + qemu_mutex_lock(&mon->common.mon_lock); > + mon->common.out_watch = > + qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > + monitor_unblocked, &mon->common); Bad indentation. Better: mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, monitor_unblocked, &mon->common); or mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, monitor_unblocked, &mon->common); or 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; > +} This function copies from monitor_data_init(), monitor_init_qmp(), and monitor_flush_locked(). Feels like a refactoring would be in order. Possibly on top. > + > +static void monitor_qmp_set_handlers_bh(void *opaque) > { > MonitorQMP *mon = opaque; > GMainContext *context; > @@ -490,7 +519,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); > } > > @@ -531,7 +567,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); > } > }
On Tue, Apr 13, 2021 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote: > > Li Zhang <zhlcindy@gmail.com> writes: > > > From: Li Zhang <li.zhang@cloud.ionos.com> > > > > 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> > > I think what what you're trying to say is that chardev-change does not > work when the character device changes is used by a QMP monitor. > Correct? > I mean that when the character device is a monitor device, it doesn't work with a QMP monitor. For example, I add 2 QMP monitors and change one of the monitor's backends from socket to a null device. It doesn't work because it needs the monitor device to support chardev-change. > If yes, how exactly does it misbehave? This command chardev-change needs specific device's change callback function. > > Does it work with an HMP monitor? No, it doesn't work. > > > --- > > monitor/monitor-internal.h | 3 +++ > > monitor/monitor.c | 2 +- > > monitor/qmp.c | 42 +++++++++++++++++++++++++++++++++++--- > > 3 files changed, 43 insertions(+), 4 deletions(-) > > > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > > index 40903d6386..2df6dd21de 100644 > > --- a/monitor/monitor-internal.h > > +++ b/monitor/monitor-internal.h > > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list); > > void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, > > Error **errp); > > > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > > + void *opaque); > > + > > #endif > > diff --git a/monitor/monitor.c b/monitor/monitor.c > > index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; > > > > @@ -480,7 +481,35 @@ 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); > > + } > > + > > + if (mon->common.out_watch) { > > + g_source_remove(mon->common.out_watch); > > All other updates of @out_watch are under @mon_lock. Why not this one? Sorry, I missed it. I will correct it. > > I have no idea whether g_source_remove() is the right function to call. > Its documentation says "You must use g_source_destroy() for sources > added to a non-default main context." The qemu_chr_fe_set_handlers() > contract is of no help. > > Documentation of g_source_destroy() confuses some more: "This does not > unref the GSource: if you still hold a reference, use g_source_unref() > to drop it. > > Marc-André, can you help? > > > + qemu_mutex_lock(&mon->common.mon_lock); > > + mon->common.out_watch = > > + qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > > + monitor_unblocked, &mon->common); > > Bad indentation. Better: > > mon->common.out_watch = > qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > monitor_unblocked, &mon->common); > > or > > mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr, > G_IO_OUT | G_IO_HUP, > monitor_unblocked, > &mon->common); > > or > > 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; > > +} > I will correct it. Thanks. > This function copies from monitor_data_init(), monitor_init_qmp(), and > monitor_flush_locked(). Feels like a refactoring would be in order. > Possibly on top. > OK, I am considering it to avoid duplicated code. > > + > > +static void monitor_qmp_set_handlers_bh(void *opaque) > > { > > MonitorQMP *mon = opaque; > > GMainContext *context; > > @@ -490,7 +519,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); > > } > > > > @@ -531,7 +567,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
On Tue, Apr 13, 2021 at 08:40:59AM +0200, Markus Armbruster wrote: > Li Zhang <zhlcindy@gmail.com> writes: > > > From: Li Zhang <li.zhang@cloud.ionos.com> > > > > 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". If you need ability to hot-add monitor instances, why not just implement that feature directly, instead of pre-creating monitors with null backends and then later changing the backend ? > > > > So this patch is to support monitor chardev hotswap with QMP. > > > > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com> > > I think what what you're trying to say is that chardev-change does not > work when the character device changes is used by a QMP monitor. > Correct? > > If yes, how exactly does it misbehave? > > Does it work with an HMP monitor? > > > --- > > monitor/monitor-internal.h | 3 +++ > > monitor/monitor.c | 2 +- > > monitor/qmp.c | 42 +++++++++++++++++++++++++++++++++++--- > > 3 files changed, 43 insertions(+), 4 deletions(-) > > > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > > index 40903d6386..2df6dd21de 100644 > > --- a/monitor/monitor-internal.h > > +++ b/monitor/monitor-internal.h > > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list); > > void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, > > Error **errp); > > > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > > + void *opaque); > > + > > #endif > > diff --git a/monitor/monitor.c b/monitor/monitor.c > > index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; > > > > @@ -480,7 +481,35 @@ 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); > > + } > > + > > + if (mon->common.out_watch) { > > + g_source_remove(mon->common.out_watch); > > All other updates of @out_watch are under @mon_lock. Why not this one? > > I have no idea whether g_source_remove() is the right function to call. > Its documentation says "You must use g_source_destroy() for sources > added to a non-default main context." The qemu_chr_fe_set_handlers() > contract is of no help. > > Documentation of g_source_destroy() confuses some more: "This does not > unref the GSource: if you still hold a reference, use g_source_unref() > to drop it. > > Marc-André, can you help? > > > + qemu_mutex_lock(&mon->common.mon_lock); > > + mon->common.out_watch = > > + qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > > + monitor_unblocked, &mon->common); > > Bad indentation. Better: > > mon->common.out_watch = > qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > monitor_unblocked, &mon->common); > > or > > mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr, > G_IO_OUT | G_IO_HUP, > monitor_unblocked, > &mon->common); > > or > > 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; > > +} > > This function copies from monitor_data_init(), monitor_init_qmp(), and > monitor_flush_locked(). Feels like a refactoring would be in order. > Possibly on top. > > > + > > +static void monitor_qmp_set_handlers_bh(void *opaque) > > { > > MonitorQMP *mon = opaque; > > GMainContext *context; > > @@ -490,7 +519,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); > > } > > > > @@ -531,7 +567,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); > > } > > } > > Regards, Daniel
On Tue, Apr 13, 2021 at 10:58 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Apr 13, 2021 at 08:40:59AM +0200, Markus Armbruster wrote: > > Li Zhang <zhlcindy@gmail.com> writes: > > > > > From: Li Zhang <li.zhang@cloud.ionos.com> > > > > > > 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". > > If you need ability to hot-add monitor instances, why not just > implement that feature directly, instead of pre-creating monitors > with null backends and then later changing the backend ? > Hi Daniel, I would like to use this solution first because we'd like to use it as soon as possible. It's the best solution to implement the hot-add feature and it may need more effort to do it. I will consider it. > > > > > > So this patch is to support monitor chardev hotswap with QMP. > > > > > > Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com> > > > > I think what what you're trying to say is that chardev-change does not > > work when the character device changes is used by a QMP monitor. > > Correct? > > > > If yes, how exactly does it misbehave? > > > > Does it work with an HMP monitor? > > > > > --- > > > monitor/monitor-internal.h | 3 +++ > > > monitor/monitor.c | 2 +- > > > monitor/qmp.c | 42 +++++++++++++++++++++++++++++++++++--- > > > 3 files changed, 43 insertions(+), 4 deletions(-) > > > > > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > > > index 40903d6386..2df6dd21de 100644 > > > --- a/monitor/monitor-internal.h > > > +++ b/monitor/monitor-internal.h > > > @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list); > > > void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, > > > Error **errp); > > > > > > +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > > > + void *opaque); > > > + > > > #endif > > > diff --git a/monitor/monitor.c b/monitor/monitor.c > > > index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; > > > > > > @@ -480,7 +481,35 @@ 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); > > > + } > > > + > > > + if (mon->common.out_watch) { > > > + g_source_remove(mon->common.out_watch); > > > > All other updates of @out_watch are under @mon_lock. Why not this one? > > > > I have no idea whether g_source_remove() is the right function to call. > > Its documentation says "You must use g_source_destroy() for sources > > added to a non-default main context." The qemu_chr_fe_set_handlers() > > contract is of no help. > > > > Documentation of g_source_destroy() confuses some more: "This does not > > unref the GSource: if you still hold a reference, use g_source_unref() > > to drop it. > > > > Marc-André, can you help? > > > > > + qemu_mutex_lock(&mon->common.mon_lock); > > > + mon->common.out_watch = > > > + qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > > > + monitor_unblocked, &mon->common); > > > > Bad indentation. Better: > > > > mon->common.out_watch = > > qemu_chr_fe_add_watch(&mon->common.chr, G_IO_OUT | G_IO_HUP, > > monitor_unblocked, &mon->common); > > > > or > > > > mon->common.out_watch = qemu_chr_fe_add_watch(&mon->common.chr, > > G_IO_OUT | G_IO_HUP, > > monitor_unblocked, > > &mon->common); > > > > or > > > > 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; > > > +} > > > > This function copies from monitor_data_init(), monitor_init_qmp(), and > > monitor_flush_locked(). Feels like a refactoring would be in order. > > Possibly on top. > > > > > + > > > +static void monitor_qmp_set_handlers_bh(void *opaque) > > > { > > > MonitorQMP *mon = opaque; > > > GMainContext *context; > > > @@ -490,7 +519,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); > > > } > > > > > > @@ -531,7 +567,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); > > > } > > > } > > > > > > 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 :| >
Li Zhang <zhlcindy@gmail.com> writes: > On Tue, Apr 13, 2021 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> Li Zhang <zhlcindy@gmail.com> writes: >> >> > From: Li Zhang <li.zhang@cloud.ionos.com> >> > >> > 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> >> >> I think what what you're trying to say is that chardev-change does not >> work when the character device changes is used by a QMP monitor. >> Correct? >> > I mean that when the character device is a monitor device, it doesn't > work with a QMP monitor. > For example, I add 2 QMP monitors and change one of the monitor's > backends from socket to a null device. > It doesn't work because it needs the monitor device to support chardev-change. > >> If yes, how exactly does it misbehave? > This command chardev-change needs specific device's change callback function. Yes, but what happens if you try anyway? I'm asking, because I'd like the answer to be worked into the commit message. >> Does it work with an HMP monitor? > No, it doesn't work.
When I tried chardev-change, it reports this error:
{
"error": {
"class": "GenericError",
"desc": "Chardev user does not support chardev hotswap"
}
}
On Fri, Apr 16, 2021 at 11:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Li Zhang <zhlcindy@gmail.com> writes:
>
> > On Tue, Apr 13, 2021 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Li Zhang <zhlcindy@gmail.com> writes:
> >>
> >> > From: Li Zhang <li.zhang@cloud.ionos.com>
> >> >
> >> > 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>
> >>
> >> I think what what you're trying to say is that chardev-change does not
> >> work when the character device changes is used by a QMP monitor.
> >> Correct?
> >>
> > I mean that when the character device is a monitor device, it doesn't
> > work with a QMP monitor.
> > For example, I add 2 QMP monitors and change one of the monitor's
> > backends from socket to a null device.
> > It doesn't work because it needs the monitor device to support chardev-change.
> >
> >> If yes, how exactly does it misbehave?
> > This command chardev-change needs specific device's change callback function.
>
> Yes, but what happens if you try anyway? I'm asking, because I'd like
> the answer to be worked into the commit message.
>
> >> Does it work with an HMP monitor?
> > No, it doesn't work.
>
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 40903d6386..2df6dd21de 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -186,4 +186,7 @@ int hmp_compare_cmd(const char *name, const char *list); void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, Error **errp); +gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, + void *opaque); + #endif diff --git a/monitor/monitor.c b/monitor/monitor.c index e94f532cf5..2d255bab18 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 2326bd7f9b..55cfb230d9 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; @@ -480,7 +481,35 @@ 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); + } + + if (mon->common.out_watch) { + g_source_remove(mon->common.out_watch); + qemu_mutex_lock(&mon->common.mon_lock); + 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; @@ -490,7 +519,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); } @@ -531,7 +567,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); } }