* [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start [not found] <20200428085122.1621-1-fengli@smarx.com> @ 2020-05-22 2:55 ` Li Feng 2020-05-22 8:58 ` Marc-André Lureau [not found] ` <20200615084341.GA25038@dimastep-nix> 0 siblings, 2 replies; 4+ messages in thread From: Li Feng @ 2020-05-22 2:55 UTC (permalink / raw) To: kyle, lifeng1519, dimastep, marcandre.lureau, Paolo Bonzini, open list:All patches CC here Cc: Li Feng When the disconnect event is triggered in the connecting stage, the tcp_chr_disconnect_locked may be called twice. The first call: #0 qemu_chr_socket_restart_timer (chr=0x55555582ee90) at chardev/char-socket.c:120 #1 0x000055555558e38c in tcp_chr_disconnect_locked (chr=<optimized out>) at chardev/char-socket.c:490 #2 0x000055555558e3cd in tcp_chr_disconnect (chr=0x55555582ee90) at chardev/char-socket.c:497 #3 0x000055555558ea32 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 #4 0x000055555558eeb8 in qemu_chr_socket_connected (task=0x55555582f300, opaque=<optimized out>) at chardev/char-socket.c:1090 #5 0x0000555555574352 in qio_task_complete (task=task@entry=0x55555582f300) at io/task.c:196 #6 0x00005555555745f4 in qio_task_thread_result (opaque=0x55555582f300) at io/task.c:111 #7 qio_task_wait_thread (task=0x55555582f300) at io/task.c:190 #8 0x000055555558f17e in tcp_chr_wait_connected (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at chardev/char-socket.c:1013 #9 0x0000555555567cbd in char_socket_client_reconnect_test (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1152 The second call: #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6 #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6 #2 0x00007ffff5abc096 in __assert_fail_base () from /lib64/libc.so.6 #3 0x00007ffff5abc142 in __assert_fail () from /lib64/libc.so.6 #4 0x000055555558d10a in qemu_chr_socket_restart_timer (chr=0x55555582ee90) at chardev/char-socket.c:125 #5 0x000055555558df0c in tcp_chr_disconnect_locked (chr=<optimized out>) at chardev/char-socket.c:490 #6 0x000055555558df4d in tcp_chr_disconnect (chr=0x55555582ee90) at chardev/char-socket.c:497 #7 0x000055555558e5b2 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 #8 0x000055555558e93a in tcp_chr_connect_client_sync (chr=chr@entry=0x55555582ee90, errp=errp@entry=0x7fffffffd178) at chardev/char-socket.c:944 #9 0x000055555558ec78 in tcp_chr_wait_connected (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at chardev/char-socket.c:1035 #10 0x000055555556804b in char_socket_client_test (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1023 Run test/test-char to reproduce this issue. test-char: chardev/char-socket.c:125: qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed. Signed-off-by: Li Feng <fengli@smartx.com> --- v5: - rebase to master v4: - remove the wrong patch - fix the char_socket_ping_pong to support the reconnect exception test v3: - add a patch to fix a crash when recvmsg return 0 - make the tests reproduce the two crash v2: - add unit test chardev/char-socket.c | 2 +- tests/test-char.c | 73 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 57 insertions(+), 18 deletions(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index e77699db48..8af7fdce88 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -486,7 +486,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr) if (emit_close) { qemu_chr_be_event(chr, CHR_EVENT_CLOSED); } - if (s->reconnect_time) { + if (s->reconnect_time && !s->reconnect_timer) { qemu_chr_socket_restart_timer(chr); } } diff --git a/tests/test-char.c b/tests/test-char.c index 3afc9b1b8d..73ba1cf601 100644 --- a/tests/test-char.c +++ b/tests/test-char.c @@ -625,12 +625,14 @@ static void char_udp_test(void) typedef struct { int event; bool got_pong; + CharBackend *be; } CharSocketTestData; #define SOCKET_PING "Hello" #define SOCKET_PONG "World" +typedef void (*char_socket_cb)(void *opaque, QEMUChrEvent event); static void char_socket_event(void *opaque, QEMUChrEvent event) @@ -639,6 +641,27 @@ char_socket_event(void *opaque, QEMUChrEvent event) data->event = event; } +static void +char_socket_event_with_error(void *opaque, QEMUChrEvent event) +{ + static bool first_error; + CharSocketTestData *data = opaque; + CharBackend *be = data->be; + data->event = event; + switch (event) { + case CHR_EVENT_OPENED: + if (!first_error) { + first_error = true; + qemu_chr_fe_disconnect(be); + } + return; + case CHR_EVENT_CLOSED: + return; + default: + return; + } +} + static void char_socket_read(void *opaque, const uint8_t *buf, int size) @@ -699,19 +722,24 @@ char_socket_addr_to_opt_str(SocketAddress *addr, bool fd_pass, } -static void -char_socket_ping_pong(QIOChannel *ioc) +static int +char_socket_ping_pong(QIOChannel *ioc, Error **errp) { char greeting[sizeof(SOCKET_PING)]; const char *response = SOCKET_PONG; - qio_channel_read_all(ioc, greeting, sizeof(greeting), &error_abort); + int ret; + ret = qio_channel_read_all(ioc, greeting, sizeof(greeting), errp); + if (ret != 0) { + object_unref(OBJECT(ioc)); + return -1; + } g_assert(memcmp(greeting, SOCKET_PING, sizeof(greeting)) == 0); - qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), &error_abort); - + qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), errp); object_unref(OBJECT(ioc)); + return 0; } @@ -723,7 +751,7 @@ char_socket_server_client_thread(gpointer data) qio_channel_socket_connect_sync(ioc, addr, &error_abort); - char_socket_ping_pong(QIO_CHANNEL(ioc)); + char_socket_ping_pong(QIO_CHANNEL(ioc), &error_abort); return NULL; } @@ -783,6 +811,7 @@ static void char_socket_server_test(gconstpointer opaque) reconnect: data.event = -1; + data.be = &be; qemu_chr_fe_set_handlers(&be, NULL, NULL, char_socket_event, NULL, &data, NULL, true); @@ -855,10 +884,13 @@ char_socket_client_server_thread(gpointer data) QIOChannelSocket *ioc = data; QIOChannelSocket *cioc; +retry: cioc = qio_channel_socket_accept(ioc, &error_abort); g_assert_nonnull(cioc); - char_socket_ping_pong(QIO_CHANNEL(cioc)); + if (char_socket_ping_pong(QIO_CHANNEL(cioc), NULL) != 0) { + goto retry; + } return NULL; } @@ -869,12 +901,13 @@ typedef struct { const char *reconnect; bool wait_connected; bool fd_pass; + char_socket_cb event_cb; } CharSocketClientTestConfig; - static void char_socket_client_test(gconstpointer opaque) { const CharSocketClientTestConfig *config = opaque; + const char_socket_cb event_cb = config->event_cb; QIOChannelSocket *ioc; char *optstr; Chardev *chr; @@ -938,8 +971,9 @@ static void char_socket_client_test(gconstpointer opaque) reconnect: data.event = -1; + data.be = &be; qemu_chr_fe_set_handlers(&be, NULL, NULL, - char_socket_event, NULL, + event_cb, NULL, &data, NULL, true); if (config->reconnect) { g_assert(data.event == -1); @@ -977,7 +1011,7 @@ static void char_socket_client_test(gconstpointer opaque) /* Setup a callback to receive the reply to our greeting */ qemu_chr_fe_set_handlers(&be, char_socket_can_read, char_socket_read, - char_socket_event, NULL, + event_cb, NULL, &data, NULL, true); g_assert(data.event == CHR_EVENT_OPENED); data.event = -1; @@ -1422,17 +1456,20 @@ int main(int argc, char **argv) #define SOCKET_CLIENT_TEST(name, addr) \ static CharSocketClientTestConfig client1 ## name = \ - { addr, NULL, false, false }; \ + { addr, NULL, false, false, char_socket_event}; \ static CharSocketClientTestConfig client2 ## name = \ - { addr, NULL, true, false }; \ + { addr, NULL, true, false, char_socket_event }; \ static CharSocketClientTestConfig client3 ## name = \ - { addr, ",reconnect=1", false }; \ + { addr, ",reconnect=1", false, false, char_socket_event }; \ static CharSocketClientTestConfig client4 ## name = \ - { addr, ",reconnect=1", true }; \ + { addr, ",reconnect=1", true, false, char_socket_event }; \ static CharSocketClientTestConfig client5 ## name = \ - { addr, NULL, false, true }; \ + { addr, NULL, false, true, char_socket_event }; \ static CharSocketClientTestConfig client6 ## name = \ - { addr, NULL, true, true }; \ + { addr, NULL, true, true, char_socket_event }; \ + static CharSocketClientTestConfig client7 ## name = \ + { addr, ",reconnect=1", true, false, \ + char_socket_event_with_error }; \ g_test_add_data_func("/char/socket/client/mainloop/" # name, \ &client1 ##name, char_socket_client_test); \ g_test_add_data_func("/char/socket/client/wait-conn/" # name, \ @@ -1444,7 +1481,9 @@ int main(int argc, char **argv) g_test_add_data_func("/char/socket/client/mainloop-fdpass/" # name, \ &client5 ##name, char_socket_client_test); \ g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \ - &client6 ##name, char_socket_client_test) + &client6 ##name, char_socket_client_test); \ + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \ + &client7 ##name, char_socket_client_test) if (has_ipv4) { SOCKET_SERVER_TEST(tcp, &tcpaddr); -- 2.11.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start 2020-05-22 2:55 ` [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng @ 2020-05-22 8:58 ` Marc-André Lureau [not found] ` <20200615084341.GA25038@dimastep-nix> 1 sibling, 0 replies; 4+ messages in thread From: Marc-André Lureau @ 2020-05-22 8:58 UTC (permalink / raw) To: Li Feng Cc: Feng Li, Dima Stepanov, Kyle Zhang, open list:All patches CC here, Paolo Bonzini On Fri, May 22, 2020 at 4:56 AM Li Feng <fengli@smartx.com> wrote: > > When the disconnect event is triggered in the connecting stage, > the tcp_chr_disconnect_locked may be called twice. > > The first call: > #0 qemu_chr_socket_restart_timer (chr=0x55555582ee90) at chardev/char-socket.c:120 > #1 0x000055555558e38c in tcp_chr_disconnect_locked (chr=<optimized out>) at chardev/char-socket.c:490 > #2 0x000055555558e3cd in tcp_chr_disconnect (chr=0x55555582ee90) at chardev/char-socket.c:497 > #3 0x000055555558ea32 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 > #4 0x000055555558eeb8 in qemu_chr_socket_connected (task=0x55555582f300, opaque=<optimized out>) at chardev/char-socket.c:1090 > #5 0x0000555555574352 in qio_task_complete (task=task@entry=0x55555582f300) at io/task.c:196 > #6 0x00005555555745f4 in qio_task_thread_result (opaque=0x55555582f300) at io/task.c:111 > #7 qio_task_wait_thread (task=0x55555582f300) at io/task.c:190 > #8 0x000055555558f17e in tcp_chr_wait_connected (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at chardev/char-socket.c:1013 > #9 0x0000555555567cbd in char_socket_client_reconnect_test (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1152 > The second call: > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6 > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6 > #2 0x00007ffff5abc096 in __assert_fail_base () from /lib64/libc.so.6 > #3 0x00007ffff5abc142 in __assert_fail () from /lib64/libc.so.6 > #4 0x000055555558d10a in qemu_chr_socket_restart_timer (chr=0x55555582ee90) at chardev/char-socket.c:125 > #5 0x000055555558df0c in tcp_chr_disconnect_locked (chr=<optimized out>) at chardev/char-socket.c:490 > #6 0x000055555558df4d in tcp_chr_disconnect (chr=0x55555582ee90) at chardev/char-socket.c:497 > #7 0x000055555558e5b2 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 > #8 0x000055555558e93a in tcp_chr_connect_client_sync (chr=chr@entry=0x55555582ee90, errp=errp@entry=0x7fffffffd178) at chardev/char-socket.c:944 > #9 0x000055555558ec78 in tcp_chr_wait_connected (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at chardev/char-socket.c:1035 > #10 0x000055555556804b in char_socket_client_test (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1023 > > Run test/test-char to reproduce this issue. > > test-char: chardev/char-socket.c:125: qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed. > > Signed-off-by: Li Feng <fengli@smartx.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > v5: > - rebase to master > > v4: > - remove the wrong patch > - fix the char_socket_ping_pong to support the reconnect exception test > > v3: > - add a patch to fix a crash when recvmsg return 0 > - make the tests reproduce the two crash > > v2: > - add unit test > > chardev/char-socket.c | 2 +- > tests/test-char.c | 73 +++++++++++++++++++++++++++++++++++++++------------ > 2 files changed, 57 insertions(+), 18 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index e77699db48..8af7fdce88 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -486,7 +486,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr) > if (emit_close) { > qemu_chr_be_event(chr, CHR_EVENT_CLOSED); > } > - if (s->reconnect_time) { > + if (s->reconnect_time && !s->reconnect_timer) { > qemu_chr_socket_restart_timer(chr); > } > } > diff --git a/tests/test-char.c b/tests/test-char.c > index 3afc9b1b8d..73ba1cf601 100644 > --- a/tests/test-char.c > +++ b/tests/test-char.c > @@ -625,12 +625,14 @@ static void char_udp_test(void) > typedef struct { > int event; > bool got_pong; > + CharBackend *be; > } CharSocketTestData; > > > #define SOCKET_PING "Hello" > #define SOCKET_PONG "World" > > +typedef void (*char_socket_cb)(void *opaque, QEMUChrEvent event); > > static void > char_socket_event(void *opaque, QEMUChrEvent event) > @@ -639,6 +641,27 @@ char_socket_event(void *opaque, QEMUChrEvent event) > data->event = event; > } > > +static void > +char_socket_event_with_error(void *opaque, QEMUChrEvent event) > +{ > + static bool first_error; > + CharSocketTestData *data = opaque; > + CharBackend *be = data->be; > + data->event = event; > + switch (event) { > + case CHR_EVENT_OPENED: > + if (!first_error) { > + first_error = true; > + qemu_chr_fe_disconnect(be); > + } > + return; > + case CHR_EVENT_CLOSED: > + return; > + default: > + return; > + } > +} > + > > static void > char_socket_read(void *opaque, const uint8_t *buf, int size) > @@ -699,19 +722,24 @@ char_socket_addr_to_opt_str(SocketAddress *addr, bool fd_pass, > } > > > -static void > -char_socket_ping_pong(QIOChannel *ioc) > +static int > +char_socket_ping_pong(QIOChannel *ioc, Error **errp) > { > char greeting[sizeof(SOCKET_PING)]; > const char *response = SOCKET_PONG; > > - qio_channel_read_all(ioc, greeting, sizeof(greeting), &error_abort); > + int ret; > + ret = qio_channel_read_all(ioc, greeting, sizeof(greeting), errp); > + if (ret != 0) { > + object_unref(OBJECT(ioc)); > + return -1; > + } > > g_assert(memcmp(greeting, SOCKET_PING, sizeof(greeting)) == 0); > > - qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), &error_abort); > - > + qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), errp); > object_unref(OBJECT(ioc)); > + return 0; > } > > > @@ -723,7 +751,7 @@ char_socket_server_client_thread(gpointer data) > > qio_channel_socket_connect_sync(ioc, addr, &error_abort); > > - char_socket_ping_pong(QIO_CHANNEL(ioc)); > + char_socket_ping_pong(QIO_CHANNEL(ioc), &error_abort); > > return NULL; > } > @@ -783,6 +811,7 @@ static void char_socket_server_test(gconstpointer opaque) > > reconnect: > data.event = -1; > + data.be = &be; > qemu_chr_fe_set_handlers(&be, NULL, NULL, > char_socket_event, NULL, > &data, NULL, true); > @@ -855,10 +884,13 @@ char_socket_client_server_thread(gpointer data) > QIOChannelSocket *ioc = data; > QIOChannelSocket *cioc; > > +retry: > cioc = qio_channel_socket_accept(ioc, &error_abort); > g_assert_nonnull(cioc); > > - char_socket_ping_pong(QIO_CHANNEL(cioc)); > + if (char_socket_ping_pong(QIO_CHANNEL(cioc), NULL) != 0) { > + goto retry; > + } > > return NULL; > } > @@ -869,12 +901,13 @@ typedef struct { > const char *reconnect; > bool wait_connected; > bool fd_pass; > + char_socket_cb event_cb; > } CharSocketClientTestConfig; > > - > static void char_socket_client_test(gconstpointer opaque) > { > const CharSocketClientTestConfig *config = opaque; > + const char_socket_cb event_cb = config->event_cb; > QIOChannelSocket *ioc; > char *optstr; > Chardev *chr; > @@ -938,8 +971,9 @@ static void char_socket_client_test(gconstpointer opaque) > > reconnect: > data.event = -1; > + data.be = &be; > qemu_chr_fe_set_handlers(&be, NULL, NULL, > - char_socket_event, NULL, > + event_cb, NULL, > &data, NULL, true); > if (config->reconnect) { > g_assert(data.event == -1); > @@ -977,7 +1011,7 @@ static void char_socket_client_test(gconstpointer opaque) > /* Setup a callback to receive the reply to our greeting */ > qemu_chr_fe_set_handlers(&be, char_socket_can_read, > char_socket_read, > - char_socket_event, NULL, > + event_cb, NULL, > &data, NULL, true); > g_assert(data.event == CHR_EVENT_OPENED); > data.event = -1; > @@ -1422,17 +1456,20 @@ int main(int argc, char **argv) > > #define SOCKET_CLIENT_TEST(name, addr) \ > static CharSocketClientTestConfig client1 ## name = \ > - { addr, NULL, false, false }; \ > + { addr, NULL, false, false, char_socket_event}; \ > static CharSocketClientTestConfig client2 ## name = \ > - { addr, NULL, true, false }; \ > + { addr, NULL, true, false, char_socket_event }; \ > static CharSocketClientTestConfig client3 ## name = \ > - { addr, ",reconnect=1", false }; \ > + { addr, ",reconnect=1", false, false, char_socket_event }; \ > static CharSocketClientTestConfig client4 ## name = \ > - { addr, ",reconnect=1", true }; \ > + { addr, ",reconnect=1", true, false, char_socket_event }; \ > static CharSocketClientTestConfig client5 ## name = \ > - { addr, NULL, false, true }; \ > + { addr, NULL, false, true, char_socket_event }; \ > static CharSocketClientTestConfig client6 ## name = \ > - { addr, NULL, true, true }; \ > + { addr, NULL, true, true, char_socket_event }; \ > + static CharSocketClientTestConfig client7 ## name = \ > + { addr, ",reconnect=1", true, false, \ > + char_socket_event_with_error }; \ > g_test_add_data_func("/char/socket/client/mainloop/" # name, \ > &client1 ##name, char_socket_client_test); \ > g_test_add_data_func("/char/socket/client/wait-conn/" # name, \ > @@ -1444,7 +1481,9 @@ int main(int argc, char **argv) > g_test_add_data_func("/char/socket/client/mainloop-fdpass/" # name, \ > &client5 ##name, char_socket_client_test); \ > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \ > - &client6 ##name, char_socket_client_test) > + &client6 ##name, char_socket_client_test); \ > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \ > + &client7 ##name, char_socket_client_test) > > if (has_ipv4) { > SOCKET_SERVER_TEST(tcp, &tcpaddr); > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20200615084341.GA25038@dimastep-nix>]
* Fwd: [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start [not found] ` <20200615084341.GA25038@dimastep-nix> @ 2020-06-17 8:08 ` Li Feng 2020-06-17 8:20 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: Li Feng @ 2020-06-17 8:08 UTC (permalink / raw) To: Marc-André Lureau, Dima Stepanov, open list:All patches CC here Hi Lureau, When will this patch be merged? Thanks, Feng Li. ---------- Forwarded message --------- 发件人： Dima Stepanov <dimastep@yandex-team.ru> Date: 2020年6月15日周一 下午4:43 Subject: Re: [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start To: Li Feng <fengli@smartx.com> Hi, I don't see this change merged to upstream. Any updates on it? Thanks, Dima. On Fri, May 22, 2020 at 10:55:54AM +0800, Li Feng wrote: > When the disconnect event is triggered in the connecting stage, > the tcp_chr_disconnect_locked may be called twice. > > The first call: > #0 qemu_chr_socket_restart_timer (chr=0x55555582ee90) at chardev/char-socket.c:120 > #1 0x000055555558e38c in tcp_chr_disconnect_locked (chr=<optimized out>) at chardev/char-socket.c:490 > #2 0x000055555558e3cd in tcp_chr_disconnect (chr=0x55555582ee90) at chardev/char-socket.c:497 > #3 0x000055555558ea32 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 > #4 0x000055555558eeb8 in qemu_chr_socket_connected (task=0x55555582f300, opaque=<optimized out>) at chardev/char-socket.c:1090 > #5 0x0000555555574352 in qio_task_complete (task=task@entry=0x55555582f300) at io/task.c:196 > #6 0x00005555555745f4 in qio_task_thread_result (opaque=0x55555582f300) at io/task.c:111 > #7 qio_task_wait_thread (task=0x55555582f300) at io/task.c:190 > #8 0x000055555558f17e in tcp_chr_wait_connected (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at chardev/char-socket.c:1013 > #9 0x0000555555567cbd in char_socket_client_reconnect_test (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1152 > The second call: > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6 > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6 > #2 0x00007ffff5abc096 in __assert_fail_base () from /lib64/libc.so.6 > #3 0x00007ffff5abc142 in __assert_fail () from /lib64/libc.so.6 > #4 0x000055555558d10a in qemu_chr_socket_restart_timer (chr=0x55555582ee90) at chardev/char-socket.c:125 > #5 0x000055555558df0c in tcp_chr_disconnect_locked (chr=<optimized out>) at chardev/char-socket.c:490 > #6 0x000055555558df4d in tcp_chr_disconnect (chr=0x55555582ee90) at chardev/char-socket.c:497 > #7 0x000055555558e5b2 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 > #8 0x000055555558e93a in tcp_chr_connect_client_sync (chr=chr@entry=0x55555582ee90, errp=errp@entry=0x7fffffffd178) at chardev/char-socket.c:944 > #9 0x000055555558ec78 in tcp_chr_wait_connected (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at chardev/char-socket.c:1035 > #10 0x000055555556804b in char_socket_client_test (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1023 > > Run test/test-char to reproduce this issue. > > test-char: chardev/char-socket.c:125: qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed. > > Signed-off-by: Li Feng <fengli@smartx.com> > --- > v5: > - rebase to master > > v4: > - remove the wrong patch > - fix the char_socket_ping_pong to support the reconnect exception test > > v3: > - add a patch to fix a crash when recvmsg return 0 > - make the tests reproduce the two crash > > v2: > - add unit test > > chardev/char-socket.c | 2 +- > tests/test-char.c | 73 +++++++++++++++++++++++++++++++++++++++------------ > 2 files changed, 57 insertions(+), 18 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index e77699db48..8af7fdce88 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -486,7 +486,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr) > if (emit_close) { > qemu_chr_be_event(chr, CHR_EVENT_CLOSED); > } > - if (s->reconnect_time) { > + if (s->reconnect_time && !s->reconnect_timer) { > qemu_chr_socket_restart_timer(chr); > } > } > diff --git a/tests/test-char.c b/tests/test-char.c > index 3afc9b1b8d..73ba1cf601 100644 > --- a/tests/test-char.c > +++ b/tests/test-char.c > @@ -625,12 +625,14 @@ static void char_udp_test(void) > typedef struct { > int event; > bool got_pong; > + CharBackend *be; > } CharSocketTestData; > > > #define SOCKET_PING "Hello" > #define SOCKET_PONG "World" > > +typedef void (*char_socket_cb)(void *opaque, QEMUChrEvent event); > > static void > char_socket_event(void *opaque, QEMUChrEvent event) > @@ -639,6 +641,27 @@ char_socket_event(void *opaque, QEMUChrEvent event) > data->event = event; > } > > +static void > +char_socket_event_with_error(void *opaque, QEMUChrEvent event) > +{ > + static bool first_error; > + CharSocketTestData *data = opaque; > + CharBackend *be = data->be; > + data->event = event; > + switch (event) { > + case CHR_EVENT_OPENED: > + if (!first_error) { > + first_error = true; > + qemu_chr_fe_disconnect(be); > + } > + return; > + case CHR_EVENT_CLOSED: > + return; > + default: > + return; > + } > +} > + > > static void > char_socket_read(void *opaque, const uint8_t *buf, int size) > @@ -699,19 +722,24 @@ char_socket_addr_to_opt_str(SocketAddress *addr, bool fd_pass, > } > > > -static void > -char_socket_ping_pong(QIOChannel *ioc) > +static int > +char_socket_ping_pong(QIOChannel *ioc, Error **errp) > { > char greeting[sizeof(SOCKET_PING)]; > const char *response = SOCKET_PONG; > > - qio_channel_read_all(ioc, greeting, sizeof(greeting), &error_abort); > + int ret; > + ret = qio_channel_read_all(ioc, greeting, sizeof(greeting), errp); > + if (ret != 0) { > + object_unref(OBJECT(ioc)); > + return -1; > + } > > g_assert(memcmp(greeting, SOCKET_PING, sizeof(greeting)) == 0); > > - qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), &error_abort); > - > + qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), errp); > object_unref(OBJECT(ioc)); > + return 0; > } > > > @@ -723,7 +751,7 @@ char_socket_server_client_thread(gpointer data) > > qio_channel_socket_connect_sync(ioc, addr, &error_abort); > > - char_socket_ping_pong(QIO_CHANNEL(ioc)); > + char_socket_ping_pong(QIO_CHANNEL(ioc), &error_abort); > > return NULL; > } > @@ -783,6 +811,7 @@ static void char_socket_server_test(gconstpointer opaque) > > reconnect: > data.event = -1; > + data.be = &be; > qemu_chr_fe_set_handlers(&be, NULL, NULL, > char_socket_event, NULL, > &data, NULL, true); > @@ -855,10 +884,13 @@ char_socket_client_server_thread(gpointer data) > QIOChannelSocket *ioc = data; > QIOChannelSocket *cioc; > > +retry: > cioc = qio_channel_socket_accept(ioc, &error_abort); > g_assert_nonnull(cioc); > > - char_socket_ping_pong(QIO_CHANNEL(cioc)); > + if (char_socket_ping_pong(QIO_CHANNEL(cioc), NULL) != 0) { > + goto retry; > + } > > return NULL; > } > @@ -869,12 +901,13 @@ typedef struct { > const char *reconnect; > bool wait_connected; > bool fd_pass; > + char_socket_cb event_cb; > } CharSocketClientTestConfig; > > - > static void char_socket_client_test(gconstpointer opaque) > { > const CharSocketClientTestConfig *config = opaque; > + const char_socket_cb event_cb = config->event_cb; > QIOChannelSocket *ioc; > char *optstr; > Chardev *chr; > @@ -938,8 +971,9 @@ static void char_socket_client_test(gconstpointer opaque) > > reconnect: > data.event = -1; > + data.be = &be; > qemu_chr_fe_set_handlers(&be, NULL, NULL, > - char_socket_event, NULL, > + event_cb, NULL, > &data, NULL, true); > if (config->reconnect) { > g_assert(data.event == -1); > @@ -977,7 +1011,7 @@ static void char_socket_client_test(gconstpointer opaque) > /* Setup a callback to receive the reply to our greeting */ > qemu_chr_fe_set_handlers(&be, char_socket_can_read, > char_socket_read, > - char_socket_event, NULL, > + event_cb, NULL, > &data, NULL, true); > g_assert(data.event == CHR_EVENT_OPENED); > data.event = -1; > @@ -1422,17 +1456,20 @@ int main(int argc, char **argv) > > #define SOCKET_CLIENT_TEST(name, addr) \ > static CharSocketClientTestConfig client1 ## name = \ > - { addr, NULL, false, false }; \ > + { addr, NULL, false, false, char_socket_event}; \ > static CharSocketClientTestConfig client2 ## name = \ > - { addr, NULL, true, false }; \ > + { addr, NULL, true, false, char_socket_event }; \ > static CharSocketClientTestConfig client3 ## name = \ > - { addr, ",reconnect=1", false }; \ > + { addr, ",reconnect=1", false, false, char_socket_event }; \ > static CharSocketClientTestConfig client4 ## name = \ > - { addr, ",reconnect=1", true }; \ > + { addr, ",reconnect=1", true, false, char_socket_event }; \ > static CharSocketClientTestConfig client5 ## name = \ > - { addr, NULL, false, true }; \ > + { addr, NULL, false, true, char_socket_event }; \ > static CharSocketClientTestConfig client6 ## name = \ > - { addr, NULL, true, true }; \ > + { addr, NULL, true, true, char_socket_event }; \ > + static CharSocketClientTestConfig client7 ## name = \ > + { addr, ",reconnect=1", true, false, \ > + char_socket_event_with_error }; \ > g_test_add_data_func("/char/socket/client/mainloop/" # name, \ > &client1 ##name, char_socket_client_test); \ > g_test_add_data_func("/char/socket/client/wait-conn/" # name, \ > @@ -1444,7 +1481,9 @@ int main(int argc, char **argv) > g_test_add_data_func("/char/socket/client/mainloop-fdpass/" # name, \ > &client5 ##name, char_socket_client_test); \ > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \ > - &client6 ##name, char_socket_client_test) > + &client6 ##name, char_socket_client_test); \ > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \ > + &client7 ##name, char_socket_client_test) > > if (has_ipv4) { > SOCKET_SERVER_TEST(tcp, &tcpaddr); > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start 2020-06-17 8:08 ` Fwd: " Li Feng @ 2020-06-17 8:20 ` Marc-André Lureau 0 siblings, 0 replies; 4+ messages in thread From: Marc-André Lureau @ 2020-06-17 8:20 UTC (permalink / raw) To: Li Feng, Paolo Bonzini; +Cc: Dima Stepanov, open list:All patches CC here [-- Attachment #1: Type: text/plain, Size: 11738 bytes --] Hi On Wed, Jun 17, 2020 at 12:09 PM Li Feng <fengli@smartx.com> wrote: > Hi Lureau, > > When will this patch be merged? > > Paolo, can you queue it? thanks Thanks, Feng Li. > > ---------- Forwarded message --------- > 发件人： Dima Stepanov <dimastep@yandex-team.ru> > Date: 2020年6月15日周一 下午4:43 > Subject: Re: [PATCH v5] char-socket: initialize reconnect timer only > when the timer doesn't start > To: Li Feng <fengli@smartx.com> > > > Hi, > > I don't see this change merged to upstream. Any updates on it? > > Thanks, Dima. > > > On Fri, May 22, 2020 at 10:55:54AM +0800, Li Feng wrote: > > When the disconnect event is triggered in the connecting stage, > > the tcp_chr_disconnect_locked may be called twice. > > > > The first call: > > #0 qemu_chr_socket_restart_timer (chr=0x55555582ee90) at > chardev/char-socket.c:120 > > #1 0x000055555558e38c in tcp_chr_disconnect_locked (chr=<optimized > out>) at chardev/char-socket.c:490 > > #2 0x000055555558e3cd in tcp_chr_disconnect (chr=0x55555582ee90) at > chardev/char-socket.c:497 > > #3 0x000055555558ea32 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, > sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 > > #4 0x000055555558eeb8 in qemu_chr_socket_connected > (task=0x55555582f300, opaque=<optimized out>) at chardev/char-socket.c:1090 > > #5 0x0000555555574352 in qio_task_complete (task=task@entry=0x55555582f300) > at io/task.c:196 > > #6 0x00005555555745f4 in qio_task_thread_result > (opaque=0x55555582f300) at io/task.c:111 > > #7 qio_task_wait_thread (task=0x55555582f300) at io/task.c:190 > > #8 0x000055555558f17e in tcp_chr_wait_connected > (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at > chardev/char-socket.c:1013 > > #9 0x0000555555567cbd in char_socket_client_reconnect_test > (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1152 > > The second call: > > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6 > > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6 > > #2 0x00007ffff5abc096 in __assert_fail_base () from /lib64/libc.so.6 > > #3 0x00007ffff5abc142 in __assert_fail () from /lib64/libc.so.6 > > #4 0x000055555558d10a in qemu_chr_socket_restart_timer > (chr=0x55555582ee90) at chardev/char-socket.c:125 > > #5 0x000055555558df0c in tcp_chr_disconnect_locked (chr=<optimized > out>) at chardev/char-socket.c:490 > > #6 0x000055555558df4d in tcp_chr_disconnect (chr=0x55555582ee90) at > chardev/char-socket.c:497 > > #7 0x000055555558e5b2 in tcp_chr_new_client (chr=chr@entry=0x55555582ee90, > sioc=sioc@entry=0x55555582f0b0) at chardev/char-socket.c:892 > > #8 0x000055555558e93a in tcp_chr_connect_client_sync (chr=chr@entry=0x55555582ee90, > errp=errp@entry=0x7fffffffd178) at chardev/char-socket.c:944 > > #9 0x000055555558ec78 in tcp_chr_wait_connected > (chr=0x55555582ee90, errp=0x555555802a08 <error_abort>) at > chardev/char-socket.c:1035 > > #10 0x000055555556804b in char_socket_client_test > (opaque=0x5555557fe020 <client8unix>) at tests/test-char.c:1023 > > > > Run test/test-char to reproduce this issue. > > > > test-char: chardev/char-socket.c:125: qemu_chr_socket_restart_timer: > Assertion `!s->reconnect_timer' failed. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > --- > > v5: > > - rebase to master > > > > v4: > > - remove the wrong patch > > - fix the char_socket_ping_pong to support the reconnect exception test > > > > v3: > > - add a patch to fix a crash when recvmsg return 0 > > - make the tests reproduce the two crash > > > > v2: > > - add unit test > > > > chardev/char-socket.c | 2 +- > > tests/test-char.c | 73 > +++++++++++++++++++++++++++++++++++++++------------ > > 2 files changed, 57 insertions(+), 18 deletions(-) > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > > index e77699db48..8af7fdce88 100644 > > --- a/chardev/char-socket.c > > +++ b/chardev/char-socket.c > > @@ -486,7 +486,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr) > > if (emit_close) { > > qemu_chr_be_event(chr, CHR_EVENT_CLOSED); > > } > > - if (s->reconnect_time) { > > + if (s->reconnect_time && !s->reconnect_timer) { > > qemu_chr_socket_restart_timer(chr); > > } > > } > > diff --git a/tests/test-char.c b/tests/test-char.c > > index 3afc9b1b8d..73ba1cf601 100644 > > --- a/tests/test-char.c > > +++ b/tests/test-char.c > > @@ -625,12 +625,14 @@ static void char_udp_test(void) > > typedef struct { > > int event; > > bool got_pong; > > + CharBackend *be; > > } CharSocketTestData; > > > > > > #define SOCKET_PING "Hello" > > #define SOCKET_PONG "World" > > > > +typedef void (*char_socket_cb)(void *opaque, QEMUChrEvent event); > > > > static void > > char_socket_event(void *opaque, QEMUChrEvent event) > > @@ -639,6 +641,27 @@ char_socket_event(void *opaque, QEMUChrEvent event) > > data->event = event; > > } > > > > +static void > > +char_socket_event_with_error(void *opaque, QEMUChrEvent event) > > +{ > > + static bool first_error; > > + CharSocketTestData *data = opaque; > > + CharBackend *be = data->be; > > + data->event = event; > > + switch (event) { > > + case CHR_EVENT_OPENED: > > + if (!first_error) { > > + first_error = true; > > + qemu_chr_fe_disconnect(be); > > + } > > + return; > > + case CHR_EVENT_CLOSED: > > + return; > > + default: > > + return; > > + } > > +} > > + > > > > static void > > char_socket_read(void *opaque, const uint8_t *buf, int size) > > @@ -699,19 +722,24 @@ char_socket_addr_to_opt_str(SocketAddress *addr, > bool fd_pass, > > } > > > > > > -static void > > -char_socket_ping_pong(QIOChannel *ioc) > > +static int > > +char_socket_ping_pong(QIOChannel *ioc, Error **errp) > > { > > char greeting[sizeof(SOCKET_PING)]; > > const char *response = SOCKET_PONG; > > > > - qio_channel_read_all(ioc, greeting, sizeof(greeting), &error_abort); > > + int ret; > > + ret = qio_channel_read_all(ioc, greeting, sizeof(greeting), errp); > > + if (ret != 0) { > > + object_unref(OBJECT(ioc)); > > + return -1; > > + } > > > > g_assert(memcmp(greeting, SOCKET_PING, sizeof(greeting)) == 0); > > > > - qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), > &error_abort); > > - > > + qio_channel_write_all(ioc, response, sizeof(SOCKET_PONG), errp); > > object_unref(OBJECT(ioc)); > > + return 0; > > } > > > > > > @@ -723,7 +751,7 @@ char_socket_server_client_thread(gpointer data) > > > > qio_channel_socket_connect_sync(ioc, addr, &error_abort); > > > > - char_socket_ping_pong(QIO_CHANNEL(ioc)); > > + char_socket_ping_pong(QIO_CHANNEL(ioc), &error_abort); > > > > return NULL; > > } > > @@ -783,6 +811,7 @@ static void char_socket_server_test(gconstpointer > opaque) > > > > reconnect: > > data.event = -1; > > + data.be = &be; > > qemu_chr_fe_set_handlers(&be, NULL, NULL, > > char_socket_event, NULL, > > &data, NULL, true); > > @@ -855,10 +884,13 @@ char_socket_client_server_thread(gpointer data) > > QIOChannelSocket *ioc = data; > > QIOChannelSocket *cioc; > > > > +retry: > > cioc = qio_channel_socket_accept(ioc, &error_abort); > > g_assert_nonnull(cioc); > > > > - char_socket_ping_pong(QIO_CHANNEL(cioc)); > > + if (char_socket_ping_pong(QIO_CHANNEL(cioc), NULL) != 0) { > > + goto retry; > > + } > > > > return NULL; > > } > > @@ -869,12 +901,13 @@ typedef struct { > > const char *reconnect; > > bool wait_connected; > > bool fd_pass; > > + char_socket_cb event_cb; > > } CharSocketClientTestConfig; > > > > - > > static void char_socket_client_test(gconstpointer opaque) > > { > > const CharSocketClientTestConfig *config = opaque; > > + const char_socket_cb event_cb = config->event_cb; > > QIOChannelSocket *ioc; > > char *optstr; > > Chardev *chr; > > @@ -938,8 +971,9 @@ static void char_socket_client_test(gconstpointer > opaque) > > > > reconnect: > > data.event = -1; > > + data.be = &be; > > qemu_chr_fe_set_handlers(&be, NULL, NULL, > > - char_socket_event, NULL, > > + event_cb, NULL, > > &data, NULL, true); > > if (config->reconnect) { > > g_assert(data.event == -1); > > @@ -977,7 +1011,7 @@ static void char_socket_client_test(gconstpointer > opaque) > > /* Setup a callback to receive the reply to our greeting */ > > qemu_chr_fe_set_handlers(&be, char_socket_can_read, > > char_socket_read, > > - char_socket_event, NULL, > > + event_cb, NULL, > > &data, NULL, true); > > g_assert(data.event == CHR_EVENT_OPENED); > > data.event = -1; > > @@ -1422,17 +1456,20 @@ int main(int argc, char **argv) > > > > #define SOCKET_CLIENT_TEST(name, addr) > \ > > static CharSocketClientTestConfig client1 ## name = > \ > > - { addr, NULL, false, false }; > \ > > + { addr, NULL, false, false, char_socket_event}; > \ > > static CharSocketClientTestConfig client2 ## name = > \ > > - { addr, NULL, true, false }; > \ > > + { addr, NULL, true, false, char_socket_event }; > \ > > static CharSocketClientTestConfig client3 ## name = > \ > > - { addr, ",reconnect=1", false }; > \ > > + { addr, ",reconnect=1", false, false, char_socket_event }; > \ > > static CharSocketClientTestConfig client4 ## name = > \ > > - { addr, ",reconnect=1", true }; > \ > > + { addr, ",reconnect=1", true, false, char_socket_event }; > \ > > static CharSocketClientTestConfig client5 ## name = > \ > > - { addr, NULL, false, true }; > \ > > + { addr, NULL, false, true, char_socket_event }; > \ > > static CharSocketClientTestConfig client6 ## name = > \ > > - { addr, NULL, true, true }; > \ > > + { addr, NULL, true, true, char_socket_event }; > \ > > + static CharSocketClientTestConfig client7 ## name = > \ > > + { addr, ",reconnect=1", true, false, > \ > > + char_socket_event_with_error }; > \ > > g_test_add_data_func("/char/socket/client/mainloop/" # name, > \ > > &client1 ##name, char_socket_client_test); > \ > > g_test_add_data_func("/char/socket/client/wait-conn/" # name, > \ > > @@ -1444,7 +1481,9 @@ int main(int argc, char **argv) > > g_test_add_data_func("/char/socket/client/mainloop-fdpass/" # name, > \ > > &client5 ##name, char_socket_client_test); > \ > > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # > name, \ > > - &client6 ##name, char_socket_client_test) > > + &client6 ##name, char_socket_client_test); > \ > > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, > \ > > + &client7 ##name, char_socket_client_test) > > > > if (has_ipv4) { > > SOCKET_SERVER_TEST(tcp, &tcpaddr); > > -- > > 2.11.0 > > > > -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 15409 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-17 8:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200428085122.1621-1-fengli@smarx.com>
2020-05-22 2:55 ` [PATCH v5] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
2020-05-22 8:58 ` Marc-André Lureau
[not found] ` <20200615084341.GA25038@dimastep-nix>
2020-06-17 8:08 ` Fwd: " Li Feng
2020-06-17 8:20 ` Marc-André Lureau
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).