* [PATCH v3 RESEND 1/2] qmp: Support chardev-change
@ 2021-07-09 16:08 Li Zhang
2021-07-09 16:08 ` [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change Li Zhang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Li Zhang @ 2021-07-09 16:08 UTC (permalink / raw)
To: armbru, marcandre.lureau, pankaj.gupta, qemu-devel; +Cc: Li Zhang
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.
Signed-off-by: Li Zhang <li.zhang@ionos.com>
---
v3 -> v2:
* Rework the patch which is to add hotswap for QMP device
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
* Refactor the functions qmp_chardev_add and qmp_chardev_change
monitor/monitor-internal.h | 1 +
monitor/monitor.c | 4 +-
monitor/qmp.c | 84 +++++++++++++++++++++++++++-----------
3 files changed, 63 insertions(+), 26 deletions(-)
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..162f73119b 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -182,5 +182,6 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
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);
+void monitor_flush_locked(Monitor *mon);
#endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index b90c0f4051..1b05ef3bdb 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -154,8 +154,6 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
}
-static void monitor_flush_locked(Monitor *mon);
-
static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
void *opaque)
{
@@ -169,7 +167,7 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
}
/* Caller must hold mon->mon_lock */
-static void monitor_flush_locked(Monitor *mon)
+void monitor_flush_locked(Monitor *mon)
{
int rc;
size_t len;
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 092c527b6f..29b49ee041 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -46,6 +46,8 @@ struct QMPRequest {
typedef struct QMPRequest QMPRequest;
QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
+static void monitor_qmp_setup_handlers_bh(void *opaque);
+static void monitor_backend_init(MonitorQMP *mon, Chardev *chr);
static bool qmp_oob_enabled(MonitorQMP *mon)
{
@@ -481,6 +483,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
g_queue_free(mon->qmp_requests);
}
+static bool monitor_in_list(Monitor *mon)
+{
+ Monitor *mon_tmp;
+ QTAILQ_FOREACH(mon_tmp, &mon_list, entry) {
+ if (mon_tmp == mon) {
+ return true;
+ }
+ }
+ return false;
+}
+
+static int monitor_qmp_change(void *opaque)
+{
+ MonitorQMP *mon = opaque;
+
+ monitor_data_init(&mon->common, true, false,
+ qemu_chr_has_feature(mon->common.chr.chr,
+ QEMU_CHAR_FEATURE_GCONTEXT));
+ monitor_backend_init(mon, mon->common.chr.chr);
+ qemu_mutex_lock(&mon->common.mon_lock);
+ if (mon->common.out_watch) {
+ mon->common.out_watch = 0;
+ monitor_flush_locked(&mon->common);
+ }
+ qemu_mutex_unlock(&mon->common.mon_lock);
+
+ return 0;
+}
+
static void monitor_qmp_setup_handlers_bh(void *opaque)
{
MonitorQMP *mon = opaque;
@@ -491,30 +522,15 @@ 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_list_append(&mon->common);
-}
+ monitor_qmp_change, &mon->common, context, true);
-void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
-{
- MonitorQMP *mon = g_new0(MonitorQMP, 1);
-
- if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
- g_free(mon);
- return;
+ if (!monitor_in_list(&mon->common)) {
+ monitor_list_append(&mon->common);
}
- qemu_chr_fe_set_echo(&mon->common.chr, true);
-
- /* Note: we run QMP monitor in I/O thread when @chr supports that */
- monitor_data_init(&mon->common, true, false,
- qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
-
- mon->pretty = pretty;
-
- qemu_mutex_init(&mon->qmp_queue_lock);
- mon->qmp_requests = g_queue_new();
+}
- json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
+static void monitor_backend_init(MonitorQMP *mon, Chardev *chr)
+{
if (mon->common.use_io_thread) {
/*
* Make sure the old iowatch is gone. It's possible when
@@ -532,7 +548,29 @@ 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_list_append(&mon->common);
+ monitor_qmp_change, &mon->common, NULL, true);
+ if (!monitor_in_list(&mon->common)) {
+ monitor_list_append(&mon->common);
+ }
}
}
+
+void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
+{
+ MonitorQMP *mon = g_new0(MonitorQMP, 1);
+
+ if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
+ g_free(mon);
+ return;
+ }
+ qemu_chr_fe_set_echo(&mon->common.chr, true);
+ /* Note: we run QMP monitor in I/O thread when @chr supports that */
+ monitor_data_init(&mon->common, true, false,
+ qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
+
+ mon->pretty = pretty;
+ qemu_mutex_init(&mon->qmp_queue_lock);
+ mon->qmp_requests = g_queue_new();
+ json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
+ monitor_backend_init(mon, chr);
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change
2021-07-09 16:08 [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
@ 2021-07-09 16:08 ` Li Zhang
2021-07-14 14:56 ` Li Zhang
2021-07-27 12:08 ` Li Zhang
2021-07-14 14:56 ` [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
2021-07-27 12:08 ` Li Zhang
2 siblings, 2 replies; 6+ messages in thread
From: Li Zhang @ 2021-07-09 16:08 UTC (permalink / raw)
To: armbru, marcandre.lureau, pankaj.gupta, qemu-devel; +Cc: Li Zhang
To improve the problematic source code in qmp_chardev_change
and some redundant source code, the functions qmp_chardev_add
and qmp_chardev_change are refactored. It is mentioned in thread:
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
Signed-off-by: Li Zhang <li.zhang@ionos.com>
---
chardev/char.c | 72 ++++++++++++++++++++++++++------------------------
1 file changed, 37 insertions(+), 35 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index d959eec522..cb6f287cd2 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1028,23 +1028,10 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
return chr;
}
-ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
- Error **errp)
+static ChardevReturn *chardev_add(const char *id, Chardev *chr,
+ Error **errp)
{
- const ChardevClass *cc;
ChardevReturn *ret;
- Chardev *chr;
-
- cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
- if (!cc) {
- return NULL;
- }
-
- chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
- backend, NULL, false, errp);
- if (!chr) {
- return NULL;
- }
if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
errp)) {
@@ -1062,6 +1049,26 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
return ret;
}
+ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
+ Error **errp)
+{
+ const ChardevClass *cc;
+ Chardev *chr;
+
+ cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
+ if (!cc) {
+ return NULL;
+ }
+
+ chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
+ backend, NULL, false, errp);
+ if (!chr) {
+ return NULL;
+ }
+
+ return chardev_add(id, chr, errp);
+}
+
ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
Error **errp)
{
@@ -1070,7 +1077,6 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
Chardev *chr, *chr_new;
bool closed_sent = false;
bool handover_yank_instance;
- ChardevReturn *ret;
chr = qemu_chr_find(id);
if (!chr) {
@@ -1089,11 +1095,22 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
return NULL;
}
+ cc = CHARDEV_GET_CLASS(chr);
+ cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
+ if (!cc_new) {
+ return NULL;
+ }
+
be = chr->be;
if (!be) {
/* easy case */
- object_unparent(OBJECT(chr));
- return qmp_chardev_add(id, backend, errp);
+ chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc_new)),
+ backend, NULL, false, errp);
+ if (!chr_new) {
+ return NULL;
+ }
+
+ goto out;
}
if (!be->chr_be_change) {
@@ -1101,12 +1118,6 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
return NULL;
}
- cc = CHARDEV_GET_CLASS(chr);
- cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
- if (!cc_new) {
- return NULL;
- }
-
/*
* The new chardev should not register a yank instance if the current
* chardev has registered one already.
@@ -1147,18 +1158,9 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
*/
chr->handover_yank_instance = handover_yank_instance;
+out:
object_unparent(OBJECT(chr));
- object_property_add_child(get_chardevs_root(), chr_new->label,
- OBJECT(chr_new));
- object_unref(OBJECT(chr_new));
-
- ret = g_new0(ChardevReturn, 1);
- if (CHARDEV_IS_PTY(chr_new)) {
- ret->pty = g_strdup(chr_new->filename + 4);
- ret->has_pty = true;
- }
-
- return ret;
+ return chardev_add(id, chr_new, errp);
}
void qmp_chardev_remove(const char *id, Error **errp)
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 RESEND 1/2] qmp: Support chardev-change
2021-07-09 16:08 [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
2021-07-09 16:08 ` [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change Li Zhang
@ 2021-07-14 14:56 ` Li Zhang
2021-07-27 12:08 ` Li Zhang
2 siblings, 0 replies; 6+ messages in thread
From: Li Zhang @ 2021-07-14 14:56 UTC (permalink / raw)
To: Markus Armbruster, Marc-André Lureau, Pankaj Gupta, QEMU; +Cc: Li Zhang
Ping
On Fri, Jul 9, 2021 at 6:11 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
> 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.
>
> Signed-off-by: Li Zhang <li.zhang@ionos.com>
> ---
> v3 -> v2:
> * Rework the patch which is to add hotswap for QMP device
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
> * Refactor the functions qmp_chardev_add and qmp_chardev_change
>
> monitor/monitor-internal.h | 1 +
> monitor/monitor.c | 4 +-
> monitor/qmp.c | 84 +++++++++++++++++++++++++++-----------
> 3 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..162f73119b 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -182,5 +182,6 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
> 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);
> +void monitor_flush_locked(Monitor *mon);
>
> #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index b90c0f4051..1b05ef3bdb 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -154,8 +154,6 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
> }
>
> -static void monitor_flush_locked(Monitor *mon);
> -
> static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> void *opaque)
> {
> @@ -169,7 +167,7 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> }
>
> /* Caller must hold mon->mon_lock */
> -static void monitor_flush_locked(Monitor *mon)
> +void monitor_flush_locked(Monitor *mon)
> {
> int rc;
> size_t len;
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 092c527b6f..29b49ee041 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -46,6 +46,8 @@ struct QMPRequest {
> typedef struct QMPRequest QMPRequest;
>
> QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> +static void monitor_qmp_setup_handlers_bh(void *opaque);
> +static void monitor_backend_init(MonitorQMP *mon, Chardev *chr);
>
> static bool qmp_oob_enabled(MonitorQMP *mon)
> {
> @@ -481,6 +483,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
> g_queue_free(mon->qmp_requests);
> }
>
> +static bool monitor_in_list(Monitor *mon)
> +{
> + Monitor *mon_tmp;
> + QTAILQ_FOREACH(mon_tmp, &mon_list, entry) {
> + if (mon_tmp == mon) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static int monitor_qmp_change(void *opaque)
> +{
> + MonitorQMP *mon = opaque;
> +
> + monitor_data_init(&mon->common, true, false,
> + qemu_chr_has_feature(mon->common.chr.chr,
> + QEMU_CHAR_FEATURE_GCONTEXT));
> + monitor_backend_init(mon, mon->common.chr.chr);
> + qemu_mutex_lock(&mon->common.mon_lock);
> + if (mon->common.out_watch) {
> + mon->common.out_watch = 0;
> + monitor_flush_locked(&mon->common);
> + }
> + qemu_mutex_unlock(&mon->common.mon_lock);
> +
> + return 0;
> +}
> +
> static void monitor_qmp_setup_handlers_bh(void *opaque)
> {
> MonitorQMP *mon = opaque;
> @@ -491,30 +522,15 @@ 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_list_append(&mon->common);
> -}
> + monitor_qmp_change, &mon->common, context, true);
>
> -void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> -{
> - MonitorQMP *mon = g_new0(MonitorQMP, 1);
> -
> - if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
> - g_free(mon);
> - return;
> + if (!monitor_in_list(&mon->common)) {
> + monitor_list_append(&mon->common);
> }
> - qemu_chr_fe_set_echo(&mon->common.chr, true);
> -
> - /* Note: we run QMP monitor in I/O thread when @chr supports that */
> - monitor_data_init(&mon->common, true, false,
> - qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
> -
> - mon->pretty = pretty;
> -
> - qemu_mutex_init(&mon->qmp_queue_lock);
> - mon->qmp_requests = g_queue_new();
> +}
>
> - json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
> +static void monitor_backend_init(MonitorQMP *mon, Chardev *chr)
> +{
> if (mon->common.use_io_thread) {
> /*
> * Make sure the old iowatch is gone. It's possible when
> @@ -532,7 +548,29 @@ 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_list_append(&mon->common);
> + monitor_qmp_change, &mon->common, NULL, true);
> + if (!monitor_in_list(&mon->common)) {
> + monitor_list_append(&mon->common);
> + }
> }
> }
> +
> +void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> +{
> + MonitorQMP *mon = g_new0(MonitorQMP, 1);
> +
> + if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
> + g_free(mon);
> + return;
> + }
> + qemu_chr_fe_set_echo(&mon->common.chr, true);
> + /* Note: we run QMP monitor in I/O thread when @chr supports that */
> + monitor_data_init(&mon->common, true, false,
> + qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
> +
> + mon->pretty = pretty;
> + qemu_mutex_init(&mon->qmp_queue_lock);
> + mon->qmp_requests = g_queue_new();
> + json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
> + monitor_backend_init(mon, chr);
> +}
> --
> 2.25.1
>
--
Best Regards
-Li
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change
2021-07-09 16:08 ` [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change Li Zhang
@ 2021-07-14 14:56 ` Li Zhang
2021-07-27 12:08 ` Li Zhang
1 sibling, 0 replies; 6+ messages in thread
From: Li Zhang @ 2021-07-14 14:56 UTC (permalink / raw)
To: Markus Armbruster, Marc-André Lureau, Pankaj Gupta, QEMU; +Cc: Li Zhang
Ping
On Fri, Jul 9, 2021 at 6:11 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
> To improve the problematic source code in qmp_chardev_change
> and some redundant source code, the functions qmp_chardev_add
> and qmp_chardev_change are refactored. It is mentioned in thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
>
> Signed-off-by: Li Zhang <li.zhang@ionos.com>
> ---
> chardev/char.c | 72 ++++++++++++++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index d959eec522..cb6f287cd2 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1028,23 +1028,10 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
> return chr;
> }
>
> -ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> - Error **errp)
> +static ChardevReturn *chardev_add(const char *id, Chardev *chr,
> + Error **errp)
> {
> - const ChardevClass *cc;
> ChardevReturn *ret;
> - Chardev *chr;
> -
> - cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
> - if (!cc) {
> - return NULL;
> - }
> -
> - chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> - backend, NULL, false, errp);
> - if (!chr) {
> - return NULL;
> - }
>
> if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
> errp)) {
> @@ -1062,6 +1049,26 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> return ret;
> }
>
> +ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> + Error **errp)
> +{
> + const ChardevClass *cc;
> + Chardev *chr;
> +
> + cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
> + if (!cc) {
> + return NULL;
> + }
> +
> + chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> + backend, NULL, false, errp);
> + if (!chr) {
> + return NULL;
> + }
> +
> + return chardev_add(id, chr, errp);
> +}
> +
> ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> Error **errp)
> {
> @@ -1070,7 +1077,6 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> Chardev *chr, *chr_new;
> bool closed_sent = false;
> bool handover_yank_instance;
> - ChardevReturn *ret;
>
> chr = qemu_chr_find(id);
> if (!chr) {
> @@ -1089,11 +1095,22 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> return NULL;
> }
>
> + cc = CHARDEV_GET_CLASS(chr);
> + cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
> + if (!cc_new) {
> + return NULL;
> + }
> +
> be = chr->be;
> if (!be) {
> /* easy case */
> - object_unparent(OBJECT(chr));
> - return qmp_chardev_add(id, backend, errp);
> + chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc_new)),
> + backend, NULL, false, errp);
> + if (!chr_new) {
> + return NULL;
> + }
> +
> + goto out;
> }
>
> if (!be->chr_be_change) {
> @@ -1101,12 +1118,6 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> return NULL;
> }
>
> - cc = CHARDEV_GET_CLASS(chr);
> - cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
> - if (!cc_new) {
> - return NULL;
> - }
> -
> /*
> * The new chardev should not register a yank instance if the current
> * chardev has registered one already.
> @@ -1147,18 +1158,9 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> */
> chr->handover_yank_instance = handover_yank_instance;
>
> +out:
> object_unparent(OBJECT(chr));
> - object_property_add_child(get_chardevs_root(), chr_new->label,
> - OBJECT(chr_new));
> - object_unref(OBJECT(chr_new));
> -
> - ret = g_new0(ChardevReturn, 1);
> - if (CHARDEV_IS_PTY(chr_new)) {
> - ret->pty = g_strdup(chr_new->filename + 4);
> - ret->has_pty = true;
> - }
> -
> - return ret;
> + return chardev_add(id, chr_new, errp);
> }
>
> void qmp_chardev_remove(const char *id, Error **errp)
> --
> 2.25.1
>
--
Best Regards
-Li
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 RESEND 1/2] qmp: Support chardev-change
2021-07-09 16:08 [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
2021-07-09 16:08 ` [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change Li Zhang
2021-07-14 14:56 ` [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
@ 2021-07-27 12:08 ` Li Zhang
2 siblings, 0 replies; 6+ messages in thread
From: Li Zhang @ 2021-07-27 12:08 UTC (permalink / raw)
To: Markus Armbruster, Marc-André Lureau, Pankaj Gupta, QEMU; +Cc: Li Zhang
Hi all,
Any comments for this patch?
On Fri, Jul 9, 2021 at 6:11 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
> 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.
>
> Signed-off-by: Li Zhang <li.zhang@ionos.com>
> ---
> v3 -> v2:
> * Rework the patch which is to add hotswap for QMP device
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
> * Refactor the functions qmp_chardev_add and qmp_chardev_change
>
> monitor/monitor-internal.h | 1 +
> monitor/monitor.c | 4 +-
> monitor/qmp.c | 84 +++++++++++++++++++++++++++-----------
> 3 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..162f73119b 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -182,5 +182,6 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
> 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);
> +void monitor_flush_locked(Monitor *mon);
>
> #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index b90c0f4051..1b05ef3bdb 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -154,8 +154,6 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
> }
>
> -static void monitor_flush_locked(Monitor *mon);
> -
> static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> void *opaque)
> {
> @@ -169,7 +167,7 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> }
>
> /* Caller must hold mon->mon_lock */
> -static void monitor_flush_locked(Monitor *mon)
> +void monitor_flush_locked(Monitor *mon)
> {
> int rc;
> size_t len;
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 092c527b6f..29b49ee041 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -46,6 +46,8 @@ struct QMPRequest {
> typedef struct QMPRequest QMPRequest;
>
> QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> +static void monitor_qmp_setup_handlers_bh(void *opaque);
> +static void monitor_backend_init(MonitorQMP *mon, Chardev *chr);
>
> static bool qmp_oob_enabled(MonitorQMP *mon)
> {
> @@ -481,6 +483,35 @@ void monitor_data_destroy_qmp(MonitorQMP *mon)
> g_queue_free(mon->qmp_requests);
> }
>
> +static bool monitor_in_list(Monitor *mon)
> +{
> + Monitor *mon_tmp;
> + QTAILQ_FOREACH(mon_tmp, &mon_list, entry) {
> + if (mon_tmp == mon) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static int monitor_qmp_change(void *opaque)
> +{
> + MonitorQMP *mon = opaque;
> +
> + monitor_data_init(&mon->common, true, false,
> + qemu_chr_has_feature(mon->common.chr.chr,
> + QEMU_CHAR_FEATURE_GCONTEXT));
> + monitor_backend_init(mon, mon->common.chr.chr);
> + qemu_mutex_lock(&mon->common.mon_lock);
> + if (mon->common.out_watch) {
> + mon->common.out_watch = 0;
> + monitor_flush_locked(&mon->common);
> + }
> + qemu_mutex_unlock(&mon->common.mon_lock);
> +
> + return 0;
> +}
> +
> static void monitor_qmp_setup_handlers_bh(void *opaque)
> {
> MonitorQMP *mon = opaque;
> @@ -491,30 +522,15 @@ 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_list_append(&mon->common);
> -}
> + monitor_qmp_change, &mon->common, context, true);
>
> -void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> -{
> - MonitorQMP *mon = g_new0(MonitorQMP, 1);
> -
> - if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
> - g_free(mon);
> - return;
> + if (!monitor_in_list(&mon->common)) {
> + monitor_list_append(&mon->common);
> }
> - qemu_chr_fe_set_echo(&mon->common.chr, true);
> -
> - /* Note: we run QMP monitor in I/O thread when @chr supports that */
> - monitor_data_init(&mon->common, true, false,
> - qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
> -
> - mon->pretty = pretty;
> -
> - qemu_mutex_init(&mon->qmp_queue_lock);
> - mon->qmp_requests = g_queue_new();
> +}
>
> - json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
> +static void monitor_backend_init(MonitorQMP *mon, Chardev *chr)
> +{
> if (mon->common.use_io_thread) {
> /*
> * Make sure the old iowatch is gone. It's possible when
> @@ -532,7 +548,29 @@ 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_list_append(&mon->common);
> + monitor_qmp_change, &mon->common, NULL, true);
> + if (!monitor_in_list(&mon->common)) {
> + monitor_list_append(&mon->common);
> + }
> }
> }
> +
> +void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> +{
> + MonitorQMP *mon = g_new0(MonitorQMP, 1);
> +
> + if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
> + g_free(mon);
> + return;
> + }
> + qemu_chr_fe_set_echo(&mon->common.chr, true);
> + /* Note: we run QMP monitor in I/O thread when @chr supports that */
> + monitor_data_init(&mon->common, true, false,
> + qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
> +
> + mon->pretty = pretty;
> + qemu_mutex_init(&mon->qmp_queue_lock);
> + mon->qmp_requests = g_queue_new();
> + json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
> + monitor_backend_init(mon, chr);
> +}
> --
> 2.25.1
>
--
Best Regards
-Li
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change
2021-07-09 16:08 ` [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change Li Zhang
2021-07-14 14:56 ` Li Zhang
@ 2021-07-27 12:08 ` Li Zhang
1 sibling, 0 replies; 6+ messages in thread
From: Li Zhang @ 2021-07-27 12:08 UTC (permalink / raw)
To: Markus Armbruster, Marc-André Lureau, Pankaj Gupta, QEMU; +Cc: Li Zhang
Hi all,
Any comments for this patch?
On Fri, Jul 9, 2021 at 6:11 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
> To improve the problematic source code in qmp_chardev_change
> and some redundant source code, the functions qmp_chardev_add
> and qmp_chardev_change are refactored. It is mentioned in thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
>
> Signed-off-by: Li Zhang <li.zhang@ionos.com>
> ---
> chardev/char.c | 72 ++++++++++++++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index d959eec522..cb6f287cd2 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1028,23 +1028,10 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
> return chr;
> }
>
> -ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> - Error **errp)
> +static ChardevReturn *chardev_add(const char *id, Chardev *chr,
> + Error **errp)
> {
> - const ChardevClass *cc;
> ChardevReturn *ret;
> - Chardev *chr;
> -
> - cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
> - if (!cc) {
> - return NULL;
> - }
> -
> - chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> - backend, NULL, false, errp);
> - if (!chr) {
> - return NULL;
> - }
>
> if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
> errp)) {
> @@ -1062,6 +1049,26 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> return ret;
> }
>
> +ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> + Error **errp)
> +{
> + const ChardevClass *cc;
> + Chardev *chr;
> +
> + cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
> + if (!cc) {
> + return NULL;
> + }
> +
> + chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> + backend, NULL, false, errp);
> + if (!chr) {
> + return NULL;
> + }
> +
> + return chardev_add(id, chr, errp);
> +}
> +
> ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> Error **errp)
> {
> @@ -1070,7 +1077,6 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> Chardev *chr, *chr_new;
> bool closed_sent = false;
> bool handover_yank_instance;
> - ChardevReturn *ret;
>
> chr = qemu_chr_find(id);
> if (!chr) {
> @@ -1089,11 +1095,22 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> return NULL;
> }
>
> + cc = CHARDEV_GET_CLASS(chr);
> + cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
> + if (!cc_new) {
> + return NULL;
> + }
> +
> be = chr->be;
> if (!be) {
> /* easy case */
> - object_unparent(OBJECT(chr));
> - return qmp_chardev_add(id, backend, errp);
> + chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc_new)),
> + backend, NULL, false, errp);
> + if (!chr_new) {
> + return NULL;
> + }
> +
> + goto out;
> }
>
> if (!be->chr_be_change) {
> @@ -1101,12 +1118,6 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> return NULL;
> }
>
> - cc = CHARDEV_GET_CLASS(chr);
> - cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
> - if (!cc_new) {
> - return NULL;
> - }
> -
> /*
> * The new chardev should not register a yank instance if the current
> * chardev has registered one already.
> @@ -1147,18 +1158,9 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> */
> chr->handover_yank_instance = handover_yank_instance;
>
> +out:
> object_unparent(OBJECT(chr));
> - object_property_add_child(get_chardevs_root(), chr_new->label,
> - OBJECT(chr_new));
> - object_unref(OBJECT(chr_new));
> -
> - ret = g_new0(ChardevReturn, 1);
> - if (CHARDEV_IS_PTY(chr_new)) {
> - ret->pty = g_strdup(chr_new->filename + 4);
> - ret->has_pty = true;
> - }
> -
> - return ret;
> + return chardev_add(id, chr_new, errp);
> }
>
> void qmp_chardev_remove(const char *id, Error **errp)
> --
> 2.25.1
>
--
Best Regards
-Li
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-27 12:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09 16:08 [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
2021-07-09 16:08 ` [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change Li Zhang
2021-07-14 14:56 ` Li Zhang
2021-07-27 12:08 ` Li Zhang
2021-07-14 14:56 ` [PATCH v3 RESEND 1/2] qmp: Support chardev-change Li Zhang
2021-07-27 12:08 ` Li Zhang
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).