chardev: convert leftover glib APIs to use dedicate gcontext
v2 cover chardev: convert leftover glib APIs to use dedicate gcontext
v2:
- add r-bs
- fix patch 3 on some s->ms conversion [Marc-André]
There were existing work that tried to allow chardev to be run in a dedicated gcontext rather than the default main context/thread. Basically that work passed in the correct gcontext during g_source_attach(). However, I found something missing along the way, that some legacy glib APIs are used by chardev code which take the main context as default:
g_timeout_add_seconds
g_timeout_add
g_idle_add
To fully allow the chardevs to be run in dedicated gcontext, we need to convert all these legacy APIs into g_source_attach() calls as well, with the correct gcontext passed in.
This series tries to clean the rest of things up.
I picked up patch 1 from monitor-oob series into this series (which is a missing of chardev frontend call fix for g_source_attach()), so that this series can be a complete fix.
Please review. Thanks,
Peter Xu (3):
chardev: use backend chr context when watch for fe
chardev: let g_idle_add() be with chardev gcontext
chardev: introduce qemu_chr_timeout_add() and use
chardev/char-fe.c | 2 +- chardev/char-pty.c | 16 ++++++++-------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 2 ++ 6 files changed, 37 insertions(+), 14 deletions(-)
Signature · 1 line
2.14.3
PATCH v2 1/3 chardev: use backend chr context when watch for fe
In commit 6bbb6c0644 ("chardev: use per-dev context for io_add_watch_poll", 2017-09-22) all the chardev watches are converted to use per-chardev gcontext to support chardev to be run outside default main thread. However that's still missing one call from the frontend code. Touch that up.
--- chardev/char-fe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
chardev/char-fe.c +1 -1
@@ -356,7 +356,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond, }g_source_set_callback (src, (GSourceFunc )func, user_data,NULL ); - tag =g_source_attach (src,NULL ); + tag =g_source_attach (src, s->gcontext);g_source_unref (src);return tag;
Signature · 1 line
2.14.3
PATCH v2 2/3 chardev: let g_idle_add() be with chardev gcontext
In reply to Peter XuThe idle task will be attached to main gcontext even if the chardev backend is running in another gcontext. Fix the only caller by extending the g_idle_add() logic into the more powerful g_source_attach(). It's basically g_idle_add_full() implementation, but with the chardev's gcontext passed in.
--- chardev/char-pty.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
chardev/char-pty.c +6 -1
@@ -210,9 +210,14 @@ static void pty_chr_state(Chardev *chr, int connected) s->timer_tag =0 ; }if (!s->connected) { +GSource *source =g_idle_source_new (); +g_assert (s->open_tag ==0 ); s->connected =1 ; - s->open_tag =g_idle_add (qemu_chr_be_generic_open_func, chr); +g_source_set_callback (source, qemu_chr_be_generic_open_func, + chr,NULL ); + s->open_tag =g_source_attach (source, chr->gcontext); +g_source_unref (source); }if (!chr->gsource) { chr->gsource = io_add_watch_poll(chr, s->ioc,
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v2 2/3] chardev: let g_idle_add() be with chardev gcontext
Quoted 11 lines · Peter Xu
The idle task will be attached to main gcontext even if the chardev backend is running in another gcontext. Fix the only caller by extending the
g_idle_add()logic into the more powerfulg_source_attach(). It's basicallyg_idle_add_full()implementation, but with the chardev's gcontext passed in.Reviewed-by Marc-André Lureau <[email protected]>Signed-off-by Peter Xu <[email protected]>--- chardev/char-pty.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
PATCH v2 3/3 chardev: introduce qemu_chr_timeout_add() and use
In reply to Peter XuIt's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function qemu_chr_timeout_add() is introduced.
One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.
--- chardev/char-pty.c | 9 ++------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 2 ++ 5 files changed, 30 insertions(+), 12 deletions(-)
chardev/char-pty.c +2 -7
@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms) s->timer_tag =0 ; } -if (ms ==1000 ) { - name =g_strdup_printf ("pty-timer-secs-%s" , chr->label); - s->timer_tag =g_timeout_add_seconds (1 , pty_chr_timer, chr); - }else { - name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); - s->timer_tag =g_timeout_add (ms, pty_chr_timer, chr); - } + name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); + s->timer_tag =qemu_chr_timeout_add (chr, ms, pty_chr_timer, chr);g_source_set_name_by_id (s->timer_tag, name);g_free (name); }
chardev/char-socket.c +2 -2
@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)char *name;assert (s->connected ==0 ); - s->reconnect_timer =g_timeout_add_seconds (s->reconnect_time, - socket_reconnect_timeout, chr); + s->reconnect_timer =qemu_chr_timeout_add (chr, s->reconnect_time *1000 , + socket_reconnect_timeout, chr); name =g_strdup_printf ("chardev-socket-reconnect-%s" , chr->label);g_source_set_name_by_id (s->reconnect_timer, name);g_free (name);
chardev/char.c +20 -0
@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)qemu_chr_be_event (chr,CHR_EVENT_BREAK ); } +/* +* Add a timeout callback for the chardev (in milliseconds). Please +* use this to add timeout hook for chardev instead of g_timeout_add() +* and g_timeout_add_seconds(), to make sure the gcontext that the +* task bound to is correct. +*/ +guint qemu_chr_timeout_add (Chardev *chr,guint ms,GSourceFunc func, +void *private) +{ +GSource *source =g_timeout_source_new (ms); +guint id; + +assert (func); +g_source_set_callback (source, func, private,NULL ); + id =g_source_attach (source, chr->gcontext); +g_source_unref (source); + +return id; +} +void qemu_chr_cleanup (void ) {object_unparent (get_chardevs_root ());
hw/char/terminal3270.c +4 -3
@@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)g_source_remove (t->timer_tag); t->timer_tag =0 ; } - t->timer_tag =g_timeout_add_seconds (600 , send_timing_mark_cb, t); - + t->timer_tag =qemu_chr_timeout_add (t->chr.chr,600 *1000 , + send_timing_mark_cb, t);memcpy (&t->inv[t->in_len], buf, size); t->in_len += size;if (t->in_len <2 ) {
@@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)* char-socket.c. Once qemu receives the terminal-type of the * client, mark handshake done and trigger everything rolling again. */ - t->timer_tag =g_timeout_add_seconds (600 , send_timing_mark_cb, t); + t->timer_tag =qemu_chr_timeout_add (t->chr.chr,600 *1000 , + send_timing_mark_cb, t);break ;case CHR_EVENT_CLOSED : sch->curr_status.scsw.dstat =SCSW_DSTAT_DEVICE_END ;
include/chardev/char.h +2 -0
@@ -258,5 +258,7 @@ extern int term_escape_char;/* console.c */ void qemu_chr_parse_vc (QemuOpts *opts,ChardevBackend *backend,Error **errp); +guint qemu_chr_timeout_add (Chardev *chr,guint ms,GSourceFunc func, +void *private);#endif
Signature · 1 line
2.14.3
PATCH v2 3/3 chardev: introduce qemu_chr_timeout_add() and use
It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function qemu_chr_timeout_add() is introduced.
One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.
---
v2 -> v2.1: Sorry I forgot to do the move in char.h. Did it in this minor version.
chardev/char-pty.c | 9 ++------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 3 +++ 5 files changed, 31 insertions(+), 12 deletions(-)
chardev/char-pty.c +2 -7
@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms) s->timer_tag =0 ; } -if (ms ==1000 ) { - name =g_strdup_printf ("pty-timer-secs-%s" , chr->label); - s->timer_tag =g_timeout_add_seconds (1 , pty_chr_timer, chr); - }else { - name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); - s->timer_tag =g_timeout_add (ms, pty_chr_timer, chr); - } + name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); + s->timer_tag =qemu_chr_timeout_add (chr, ms, pty_chr_timer, chr);g_source_set_name_by_id (s->timer_tag, name);g_free (name); }
chardev/char-socket.c +2 -2
@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)char *name;assert (s->connected ==0 ); - s->reconnect_timer =g_timeout_add_seconds (s->reconnect_time, - socket_reconnect_timeout, chr); + s->reconnect_timer =qemu_chr_timeout_add (chr, s->reconnect_time *1000 , + socket_reconnect_timeout, chr); name =g_strdup_printf ("chardev-socket-reconnect-%s" , chr->label);g_source_set_name_by_id (s->reconnect_timer, name);g_free (name);
chardev/char.c +20 -0
@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)qemu_chr_be_event (chr,CHR_EVENT_BREAK ); } +/* +* Add a timeout callback for the chardev (in milliseconds). Please +* use this to add timeout hook for chardev instead of g_timeout_add() +* and g_timeout_add_seconds(), to make sure the gcontext that the +* task bound to is correct. +*/ +guint qemu_chr_timeout_add (Chardev *chr,guint ms,GSourceFunc func, +void *private) +{ +GSource *source =g_timeout_source_new (ms); +guint id; + +assert (func); +g_source_set_callback (source, func, private,NULL ); + id =g_source_attach (source, chr->gcontext); +g_source_unref (source); + +return id; +} +void qemu_chr_cleanup (void ) {object_unparent (get_chardevs_root ());
hw/char/terminal3270.c +4 -3
@@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)g_source_remove (t->timer_tag); t->timer_tag =0 ; } - t->timer_tag =g_timeout_add_seconds (600 , send_timing_mark_cb, t); - + t->timer_tag =qemu_chr_timeout_add (t->chr.chr,600 *1000 , + send_timing_mark_cb, t);memcpy (&t->inv[t->in_len], buf, size); t->in_len += size;if (t->in_len <2 ) {
@@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)* char-socket.c. Once qemu receives the terminal-type of the * client, mark handshake done and trigger everything rolling again. */ - t->timer_tag =g_timeout_add_seconds (600 , send_timing_mark_cb, t); + t->timer_tag =qemu_chr_timeout_add (t->chr.chr,600 *1000 , + send_timing_mark_cb, t);break ;case CHR_EVENT_CLOSED : sch->curr_status.scsw.dstat =SCSW_DSTAT_DEVICE_END ;
include/chardev/char.h +3 -0
@@ -256,6 +256,9 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,extern int term_escape_char; +guint qemu_chr_timeout_add (Chardev *chr,guint ms,GSourceFunc func, +void *private); +/* console.c */ void qemu_chr_parse_vc (QemuOpts *opts,ChardevBackend *backend,Error **errp);
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use
----- Original Message -----
Quoted 6 lines
It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function
qemu_chr_timeout_add()is introduced.
Quoted 6 lines
One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.
Signed-off-by Peter Xu <[email protected]>
Quoted 127 lines
---
v2 -> v2.1: Sorry I forgot to do the move in
char.h. Did it in this minor version.chardev/char-pty.c | 9 ++------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 3 +++ 5 files changed, 31 insertions(+), 12 deletions(-)chardev/char-pty.c +2
-7@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms) s->timer_tag =0 ; } -if (ms ==1000 ) { - name =g_strdup_printf ("pty-timer-secs-%s" , chr->label); - s->timer_tag =g_timeout_add_seconds (1 , pty_chr_timer, chr); - }else { - name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); - s->timer_tag =g_timeout_add (ms, pty_chr_timer, chr); - } + name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); + s->timer_tag =qemu_chr_timeout_add (chr, ms, pty_chr_timer, chr);g_source_set_name_by_id (s->timer_tag, name);g_free (name); }chardev/char-socket.c +2
-2@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)char *name;assert (s->connected ==0 ); - s->reconnect_timer =g_timeout_add_seconds (s->reconnect_time, - socket_reconnect_timeout, chr); + s->reconnect_timer =qemu_chr_timeout_add (chr, s->reconnect_time *1000 , + socket_reconnect_timeout, chr); name =g_strdup_printf ("chardev-socket-reconnect-%s" , chr->label);g_source_set_name_by_id(s->reconnect_timer, name); g_free(name);chardev/char.c +20
-0@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)qemu_chr_be_event (chr,CHR_EVENT_BREAK ); } +/* +* Add a timeout callback for the chardev (in milliseconds). Please +* use this to add timeout hook for chardev instead of g_timeout_add() +* and g_timeout_add_seconds(), to make sure the gcontext that the +* task bound to is correct. +*/ +guint qemu_chr_timeout_add (Chardev *chr,guint ms,GSourceFunc func, +void *private) +{ +GSource *source =g_timeout_source_new (ms); +guint id; + +assert (func); +g_source_set_callback (source, func, private,NULL ); + id =g_source_attach (source, chr->gcontext); +g_source_unref (source); + +return id; +} +void qemu_chr_cleanup (void ) {object_unparent(get_chardevs_root());hw/char/terminal3270.c +2
-2@@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size) g_source_remove(t->timer_tag); t->timer_tag =0 ; } - t->timer_tag =g_timeout_add_seconds (600 , send_timing_mark_cb, t); - + t->timer_tag =qemu_chr_timeout_add (t->chr.chr,600 *1000 , + send_timing_mark_cb, t);memcpy (&t->inv[t->in_len], buf, size); t->in_len += size;if (t->in_len < 2) {hw/char/terminal3270.c +2
-1@@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)* char-socket.c. Once qemu receives the terminal-type of the * client, mark handshake done and trigger everything rolling again. */ - t->timer_tag =g_timeout_add_seconds (600 , send_timing_mark_cb, t); + t->timer_tag =qemu_chr_timeout_add (t->chr.chr,600 *1000 , + send_timing_mark_cb, t);break ;case CHR_EVENT_CLOSED : sch->curr_status.scsw.dstat =SCSW_DSTAT_DEVICE_END ;include/chardev/char.h +3
-0@@ -256,6 +256,9 @@ Chardev *qemu_chardev_new(const char *id, const char *typename, extern int term_escape_char; +guint qemu_chr_timeout_add (Chardev *chr,guint ms,GSourceFunc func, +void *private); +/* console.c */ void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error**errp);
--
2.14.3
Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use
In reply to Peter XuQuoted 31 lines · Peter Xu
It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function
qemu_chr_timeout_add()is introduced.One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.
Signed-off-by Peter Xu <[email protected]>---
v2 -> v2.1: Sorry I forgot to do the move in
char.h. Did it in this minor version.chardev/char-pty.c | 9 ++------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 3 +++ 5 files changed, 31 insertions(+), 12 deletions(-)chardev/char-pty.c +0
-0@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms) s->timer_tag =0 ; }
chardev/char-pty.c static void pty_chr_rearm_timer(Chardev *chr, int ms) · line 82
Quoted 10 lines
chardev/char-pty.c +2
-7-if (ms ==1000 ) { - name =g_strdup_printf ("pty-timer-secs-%s" , chr->label); - s->timer_tag =g_timeout_add_seconds (1 , pty_chr_timer, chr); - }else { - name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); - s->timer_tag =g_timeout_add (ms, pty_chr_timer, chr); - } + name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); + s->timer_tag =qemu_chr_timeout_add (chr, ms, pty_chr_timer, chr);
The label is user-visible. Why did you remove the seconds label format?
Please either include justification in the commit description or avoid spurious changes like this so reviewers don't need to worry about code changes that are not essential.
chardev/char-socket.c static void qemu_chr_socket_restart_timer(Chardev *chr) · line 77
Quoted 13 lines
chardev/char-pty.c +0
-0g_source_set_name_by_id (s->timer_tag, name);g_free (name); }chardev/char-socket.c +0
-2@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)char *name;assert (s->connected ==0 ); - s->reconnect_timer =g_timeout_add_seconds (s->reconnect_time, - socket_reconnect_timeout, chr);
Here it was clear that reconnect_time is in seconds...
chardev/char-socket.c static void qemu_chr_socket_restart_timer(Chardev *chr) · line 77
Quoted 2 lines
chardev/char-socket.c +2
-0+ s->reconnect_timer =qemu_chr_timeout_add (chr, s->reconnect_time *1000 , + socket_reconnect_timeout, chr);
...now I can't tell what the unit is.
Please rename qemu_chr_timeout_add() to include the units:
s->reconnect_timer = qemu_chr_timeout_add_ms(chr, s->reconnect_time * 1000,
Quoted 8 lines
chardev/char-socket.c +0
-0name =g_strdup_printf ("chardev-socket-reconnect-%s" , chr->label);g_source_set_name_by_id (s->reconnect_timer, name);g_free (name);chardev/char.c +0
-0
chardev/char.c void qmp_chardev_send_break(const char *id, Error **errp) · line 1092
Quoted 9 lines
chardev/char.c +6
-0@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)qemu_chr_be_event (chr,CHR_EVENT_BREAK ); } +/* +* Add a timeout callback for the chardev (in milliseconds). Please +* use this to add timeout hook for chardev instead of g_timeout_add() +* and g_timeout_add_seconds(), to make sure the gcontext that the +* task bound to is correct. +*/
What is the return value?
Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use
Quoted 33 lines · Stefan Hajnoczi
Quoted 32 lines · Peter Xu
It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function
qemu_chr_timeout_add()is introduced.One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.
Signed-off-by Peter Xu <[email protected]>---
v2 -> v2.1: Sorry I forgot to do the move in
char.h. Did it in this minor version.chardev/char-pty.c | 9 ++------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 3 +++ 5 files changed, 31 insertions(+), 12 deletions(-)chardev/char-pty.c +0
-0@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms) s->timer_tag =0 ; }
Quoted 11 lines
chardev/char-pty.c static void pty_chr_rearm_timer(Chardev *chr, int ms) · line 82
Quoted 9 lines
chardev/char-pty.c +2
-7-if (ms ==1000 ) { - name =g_strdup_printf ("pty-timer-secs-%s" , chr->label); - s->timer_tag =g_timeout_add_seconds (1 , pty_chr_timer, chr); - }else { - name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); - s->timer_tag =g_timeout_add (ms, pty_chr_timer, chr); - } + name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); + s->timer_tag =qemu_chr_timeout_add (chr, ms, pty_chr_timer, chr);The label is user-visible. Why did you remove the seconds label format?
It's used for g_source_set_name_by_id() below, and that's not user-visible AFAICT?
I removed it because I thought it was not user visible and actually I didn't see a point on doing that. Please let me know if I made a mistake.
Quoted 4 lines
Please either include justification in the commit description or avoid spurious changes like this so reviewers don't need to worry about code changes that are not essential.
Yes. I can add this into commit message after confirmed with you on above.
Quoted 17 lines
Quoted 9 lines
chardev/char-pty.c +0
-0g_source_set_name_by_id (s->timer_tag, name);g_free (name); }chardev/char-socket.c +0
-0chardev/char-socket.c static void qemu_chr_socket_restart_timer(Chardev *chr) · line 77
Quoted 5 lines
chardev/char-socket.c +0
-2@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)char *name;assert (s->connected ==0 ); - s->reconnect_timer =g_timeout_add_seconds (s->reconnect_time, - socket_reconnect_timeout, chr);Here it was clear that
reconnect_timeis in seconds...
Quoted 8 lines
chardev/char-socket.c static void qemu_chr_socket_restart_timer(Chardev *chr) · line 77
Quoted 2 lines
chardev/char-socket.c +2
-0+ s->reconnect_timer =qemu_chr_timeout_add (chr, s->reconnect_time *1000 , + socket_reconnect_timeout, chr);...now I can't tell what the unit is.
Please rename
qemu_chr_timeout_add()to include the units:
s->reconnect_timer= qemu_chr_timeout_add_ms(chr,s->reconnect_time* 1000,
Sure.
Quoted 12 lines
Quoted 12 lines
chardev/char-socket.c +0
-0name =g_strdup_printf ("chardev-socket-reconnect-%s" , chr->label);g_source_set_name_by_id (s->reconnect_timer, name);g_free (name);chardev/char.c +0
-0@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)qemu_chr_be_event (chr,CHR_EVENT_BREAK ); }
Quoted 8 lines
chardev/char.c void qmp_chardev_send_break(const char *id, Error **errp) · line 1092
Quoted 6 lines
chardev/char.c +6
-0+/* +* Add a timeout callback for the chardev (in milliseconds). Please +* use this to add timeout hook for chardev instead of g_timeout_add() +* and g_timeout_add_seconds(), to make sure the gcontext that the +* task bound to is correct. +*/ What is the return value?
Basically I mean it's a wrapper of the other two functions so the return value would be the same. But sure I'll note that out. Thanks,
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use
Quoted 54 lines · Peter Xu
Quoted 33 lines · Stefan Hajnoczi
Quoted 32 lines · Peter Xu
It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function
qemu_chr_timeout_add()is introduced.One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.
Signed-off-by Peter Xu <[email protected]>---
v2 -> v2.1: Sorry I forgot to do the move in
char.h. Did it in this minor version.chardev/char-pty.c | 9 ++------- chardev/char-socket.c | 4 ++-- chardev/char.c | 20 ++++++++++++++++++++ hw/char/terminal3270.c | 7 ++++--- include/chardev/char.h | 3 +++ 5 files changed, 31 insertions(+), 12 deletions(-)chardev/char-pty.c +0
-0@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms) s->timer_tag =0 ; }Quoted 11 lines
chardev/char-pty.c static void pty_chr_rearm_timer(Chardev *chr, int ms) · line 82
Quoted 9 lines
chardev/char-pty.c +2
-7-if (ms ==1000 ) { - name =g_strdup_printf ("pty-timer-secs-%s" , chr->label); - s->timer_tag =g_timeout_add_seconds (1 , pty_chr_timer, chr); - }else { - name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); - s->timer_tag =g_timeout_add (ms, pty_chr_timer, chr); - } + name =g_strdup_printf ("pty-timer-ms-%s" , chr->label); + s->timer_tag =qemu_chr_timeout_add (chr, ms, pty_chr_timer, chr);The label is user-visible. Why did you remove the seconds label format?
It's used for
g_source_set_name_by_id()below, and that's not user-visible AFAICT?I removed it because I thought it was not user visible and actually I didn't see a point on doing that. Please let me know if I made a mistake.
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
Please either include justification in the commit description or avoid spurious changes like this so reviewers don't need to worry about code changes that are not essential.
Yes. I can add this into commit message after confirmed with you on above.
You are right, the GSource name isn't visible (it's only used for debugging). I was thinking of chr->label.
It's still helpful to mention that the ms == 1000 special case is not user-visible in the commit description.
Thanks,
Stefan