* Re: [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect
2020-04-15 3:28 ` [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect Li Feng
@ 2020-04-14 1:24 ` Raphael Norwitz
2020-04-20 11:18 ` Li Feng
0 siblings, 1 reply; 32+ messages in thread
From: Raphael Norwitz @ 2020-04-14 1:24 UTC (permalink / raw)
To: Li Feng; +Cc: qemu-devel, mst
On Wed, Apr 15, 2020 at 11:28:23AM +0800, Li Feng wrote:
>
> switch (event) {
> case CHR_EVENT_OPENED:
> @@ -363,7 +376,16 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> }
> break;
> case CHR_EVENT_CLOSED:
> - vhost_user_blk_disconnect(dev);
> + /*
> + * a close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear to idle.
> + */
> + ctx = qemu_get_current_aio_context();
> +
> + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL,
> + NULL, NULL, NULL, false);
> + aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
This seems a bit racy. What’s to stop the async operation from executing before
the next read?
If the issue is just that the vhost_dev state is being destroyed too early, why
don’t we rather move the vhost_dev_cleanup() call from vhost_user_blk_disconnect()
to vhost_user_blk_connect()? We may need to add some state to tell if this is the
first connect or a reconnect so we don’t call vhost_dev_cleanup() on initial
connect, but as long as we call vhost_dev_cleanup() before vhost_dev_init()
every time the device reconnects it shouldn’t matter that we keep that state
around.
Thoughts?
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] vhost-user-blk: fix invalid memory access
2020-04-15 3:28 ` [PATCH 2/4] vhost-user-blk: fix invalid memory access Li Feng
@ 2020-04-14 1:44 ` Raphael Norwitz
0 siblings, 0 replies; 32+ messages in thread
From: Raphael Norwitz @ 2020-04-14 1:44 UTC (permalink / raw)
To: Li Feng; +Cc: qemu-devel, mst
On Wed, Apr 15, 2020 at 11:28:24AM +0800, Li Feng wrote:
>
> when s->inflight is freed, vhost_dev_free_inflight may try to access
> s->inflight->addr, it will retrigger the following issue.
>
> ==7309==ERROR: AddressSanitizer: heap-use-after-free on address 0x604001020d18 at pc 0x555555ce948a bp 0x7fffffffb170 sp 0x7fffffffb160
> READ of size 8 at 0x604001020d18 thread T0
> #0 0x555555ce9489 in vhost_dev_free_inflight /root/smartx/qemu-el7/qemu-test/hw/virtio/vhost.c:1473
> #1 0x555555cd86eb in virtio_reset /root/smartx/qemu-el7/qemu-test/hw/virtio/virtio.c:1214
> #2 0x5555560d3eff in virtio_pci_reset hw/virtio/virtio-pci.c:1859
> #3 0x555555f2ac53 in device_set_realized hw/core/qdev.c:893
> #4 0x5555561d572c in property_set_bool qom/object.c:1925
> #5 0x5555561de8de in object_property_set_qobject qom/qom-qobject.c:27
> #6 0x5555561d99f4 in object_property_set_bool qom/object.c:1188
> #7 0x555555e50ae7 in qdev_device_add /root/smartx/qemu-el7/qemu-test/qdev-monitor.c:626
> #8 0x555555e51213 in qmp_device_add /root/smartx/qemu-el7/qemu-test/qdev-monitor.c:806
> #9 0x555555e8ff40 in hmp_device_add /root/smartx/qemu-el7/qemu-test/hmp.c:1951
> #10 0x555555be889a in handle_hmp_command /root/smartx/qemu-el7/qemu-test/monitor.c:3404
> #11 0x555555beac8b in monitor_command_cb /root/smartx/qemu-el7/qemu-test/monitor.c:4296
> #12 0x555556433eb7 in readline_handle_byte util/readline.c:393
> #13 0x555555be89ec in monitor_read /root/smartx/qemu-el7/qemu-test/monitor.c:4279
> #14 0x5555563285cc in tcp_chr_read chardev/char-socket.c:470
> #15 0x7ffff670b968 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4a968)
> #16 0x55555640727c in glib_pollfds_poll util/main-loop.c:215
> #17 0x55555640727c in os_host_main_loop_wait util/main-loop.c:238
> #18 0x55555640727c in main_loop_wait util/main-loop.c:497
> #19 0x555555b2d0bf in main_loop /root/smartx/qemu-el7/qemu-test/vl.c:2013
> #20 0x555555b2d0bf in main /root/smartx/qemu-el7/qemu-test/vl.c:4776
> #21 0x7fffdd2eb444 in __libc_start_main (/lib64/libc.so.6+0x22444)
> #22 0x555555b3767a (/root/smartx/qemu-el7/qemu-test/x86_64-softmmu/qemu-system-x86_64+0x5e367a)
>
> 0x604001020d18 is located 8 bytes inside of 40-byte region [0x604001020d10,0x604001020d38)
> freed by thread T0 here:
> #0 0x7ffff6f00508 in __interceptor_free (/lib64/libasan.so.4+0xde508)
> #1 0x7ffff671107d in g_free (/lib64/libglib-2.0.so.0+0x5007d)
>
> previously allocated by thread T0 here:
> #0 0x7ffff6f00a88 in __interceptor_calloc (/lib64/libasan.so.4+0xdea88)
> #1 0x7ffff6710fc5 in g_malloc0 (/lib64/libglib-2.0.so.0+0x4ffc5)
>
> SUMMARY: AddressSanitizer: heap-use-after-free /root/smartx/qemu-el7/qemu-test/hw/virtio/vhost.c:1473 in vhost_dev_free_inflight
> Shadow bytes around the buggy address:
> 0x0c08801fc150: fa fa 00 00 00 00 04 fa fa fa fd fd fd fd fd fa
> 0x0c08801fc160: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 04 fa
> 0x0c08801fc170: fa fa 00 00 00 00 00 01 fa fa 00 00 00 00 04 fa
> 0x0c08801fc180: fa fa 00 00 00 00 00 01 fa fa 00 00 00 00 00 01
> 0x0c08801fc190: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 04 fa
> =>0x0c08801fc1a0: fa fa fd[fd]fd fd fd fa fa fa fd fd fd fd fd fa
> 0x0c08801fc1b0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
> 0x0c08801fc1c0: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fd
> 0x0c08801fc1d0: fa fa 00 00 00 00 00 01 fa fa fd fd fd fd fd fa
> 0x0c08801fc1e0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fd
> 0x0c08801fc1f0: fa fa 00 00 00 00 00 01 fa fa fd fd fd fd fd fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Freed heap region: fd
> Stack left redzone: f1
> Stack mid redzone: f2
> Stack right redzone: f3
> Stack after return: f5
> Stack use after scope: f8
> Global redzone: f9
> Global init order: f6
> Poisoned by user: f7
> Container overflow: fc
> Array cookie: ac
> Intra object redzone: bb
> ASan internal: fe
> Left alloca redzone: ca
> Right alloca redzone: cb
> ==7309==ABORTING
>
> Signed-off-by: Li Feng <fengli@smartx.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/block/vhost-user-blk.c | 4 ++++
> hw/virtio/vhost.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 776b9af3eb..19e79b96e4 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -463,7 +463,9 @@ reconnect:
>
> virtio_err:
> g_free(s->vhost_vqs);
> + s->vhost_vqs = NULL;
> g_free(s->inflight);
> + s->inflight = NULL;
> for (i = 0; i < s->num_queues; i++) {
> virtio_delete_queue(s->virtqs[i]);
> }
> @@ -484,7 +486,9 @@ static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp)
> vhost_dev_cleanup(&s->dev);
> vhost_dev_free_inflight(s->inflight);
> g_free(s->vhost_vqs);
> + s->vhost_vqs = NULL;
> g_free(s->inflight);
> + s->inflight = NULL;
>
> for (i = 0; i < s->num_queues; i++) {
> virtio_delete_queue(s->virtqs[i]);
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 01ebe12f28..aff98a0ede 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1514,7 +1514,7 @@ void vhost_dev_set_config_notifier(struct vhost_dev *hdev,
>
> void vhost_dev_free_inflight(struct vhost_inflight *inflight)
> {
> - if (inflight->addr) {
> + if (inflight && inflight->addr) {
> qemu_memfd_free(inflight->addr, inflight->size, inflight->fd);
> inflight->addr = NULL;
> inflight->fd = -1;
> --
> 2.11.0
>
>
> --
> The SmartX email address is only for business purpose. Any sent message
> that is not related to the business is not authorized or permitted by
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] vhost-user-blk: fix crash in realize process
2020-04-15 3:28 ` [PATCH 4/4] vhost-user-blk: fix crash in realize process Li Feng
@ 2020-04-14 1:54 ` Raphael Norwitz
[not found] ` <CAHckoCwsLwqNhVHK4PF+pzt5ExkurPNsWLw7ueRG8dfOUA2rXg@mail.gmail.com>
0 siblings, 1 reply; 32+ messages in thread
From: Raphael Norwitz @ 2020-04-14 1:54 UTC (permalink / raw)
To: Li Feng; +Cc: qemu-devel, mst
Mostly looks good - just a few superficial notes.
On Wed, Apr 15, 2020 at 11:28:26AM +0800, Li Feng wrote:
> 1. set s->connected to true after vhost_dev_init;
> 2. call vhost_dev_get_config when s->connected is true, otherwise the
> hdev->host_ops will be nullptr.
You mean hdev->vhost_ops, right?
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> hw/block/vhost-user-blk.c | 47 +++++++++++++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 22 deletions(-)
> + /*
> + * set true util vhost_dev_init return ok, because CLOSE event may happen
> + * in vhost_dev_init routine.
> + */
I'm a little confused by this comment. Do you mean to say “wait until vhost_dev_init
succeeds to set connected to true, because a close event may happen while
vhost_dev_init is executing”?
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev
@ 2020-04-15 3:28 Li Feng
2020-04-15 3:28 ` [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect Li Feng
` (4 more replies)
0 siblings, 5 replies; 32+ messages in thread
From: Li Feng @ 2020-04-15 3:28 UTC (permalink / raw)
To: kyle, Kevin Wolf, Marc-André Lureau, Max Reitz,
Michael S. Tsirkin, Paolo Bonzini, open list:Block layer core,
open list:All patches CC here, Raphael Norwitz
Cc: Li Feng
The following patches fix various crashes happened when injecting errors to
chardev unix domain socket.
The crashes are encountered when the socket is from connected to disconnected at
vhost-user-blk realize routine.
These crashes could be reproduced like this:
1. gdb break at vhost_user_write;
2. add a vhost-user-blk device through qmp;
3. when stop at vhost_user_write, kill the vhost-user-blk target;
3. let qemu continue running;
4. start vhost-user-blk;
5. see crash!
The 'CLOSE' event path is core trouble maker.
qemu_chr_fe_set_handlers
-> vhost_user_blk_event(OPEN)
-> vhost_user_blk_connect
-> vhost_dev_init
-> vhost_user_blk_event(CLOSE)
-> vhost_dev_cleanup
Li Feng (4):
vhost-user-blk: delay vhost_user_blk_disconnect
vhost-user-blk: fix invalid memory access
char-socket: avoid double call tcp_chr_free_connection
vhost-user-blk: fix crash in realize process
chardev/char-socket.c | 5 ++++
hw/block/vhost-user-blk.c | 75 ++++++++++++++++++++++++++++++++---------------
hw/virtio/vhost.c | 2 +-
3 files changed, 58 insertions(+), 24 deletions(-)
--
2.11.0
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect
2020-04-15 3:28 [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Li Feng
@ 2020-04-15 3:28 ` Li Feng
2020-04-14 1:24 ` Raphael Norwitz
2020-04-15 3:28 ` [PATCH 2/4] vhost-user-blk: fix invalid memory access Li Feng
` (3 subsequent siblings)
4 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-04-15 3:28 UTC (permalink / raw)
To: kyle, Raphael Norwitz, Michael S. Tsirkin, Kevin Wolf, Max Reitz,
open list:Block layer core, open list:All patches CC here
Cc: Li Feng
Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
may trigger a disconnect events, calling vhost_user_blk_disconnect() and
clearing all the vhost_dev strutures. Then the next socket read will
encounter an invalid pointer to vhost_dev.
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/block/vhost-user-blk.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 17df5338e7..776b9af3eb 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -349,11 +349,24 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
vhost_dev_cleanup(&s->dev);
}
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
+
+static void vhost_user_blk_chr_closed_bh(void *opaque)
+{
+ DeviceState *dev = opaque;
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+ VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ vhost_user_blk_disconnect(dev);
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
+ NULL, (void *)dev, NULL, true);
+}
+
static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
{
DeviceState *dev = opaque;
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ AioContext *ctx;
switch (event) {
case CHR_EVENT_OPENED:
@@ -363,7 +376,16 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
}
break;
case CHR_EVENT_CLOSED:
- vhost_user_blk_disconnect(dev);
+ /*
+ * a close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear to idle.
+ */
+ ctx = qemu_get_current_aio_context();
+
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL,
+ NULL, NULL, NULL, false);
+ aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
--
2.11.0
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/4] vhost-user-blk: fix invalid memory access
2020-04-15 3:28 [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Li Feng
2020-04-15 3:28 ` [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect Li Feng
@ 2020-04-15 3:28 ` Li Feng
2020-04-14 1:44 ` Raphael Norwitz
2020-04-15 3:28 ` [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection Li Feng
` (2 subsequent siblings)
4 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-04-15 3:28 UTC (permalink / raw)
To: kyle, Raphael Norwitz, Michael S. Tsirkin, Kevin Wolf, Max Reitz,
open list:Block layer core, open list:All patches CC here
Cc: Li Feng
when s->inflight is freed, vhost_dev_free_inflight may try to access
s->inflight->addr, it will retrigger the following issue.
==7309==ERROR: AddressSanitizer: heap-use-after-free on address 0x604001020d18 at pc 0x555555ce948a bp 0x7fffffffb170 sp 0x7fffffffb160
READ of size 8 at 0x604001020d18 thread T0
#0 0x555555ce9489 in vhost_dev_free_inflight /root/smartx/qemu-el7/qemu-test/hw/virtio/vhost.c:1473
#1 0x555555cd86eb in virtio_reset /root/smartx/qemu-el7/qemu-test/hw/virtio/virtio.c:1214
#2 0x5555560d3eff in virtio_pci_reset hw/virtio/virtio-pci.c:1859
#3 0x555555f2ac53 in device_set_realized hw/core/qdev.c:893
#4 0x5555561d572c in property_set_bool qom/object.c:1925
#5 0x5555561de8de in object_property_set_qobject qom/qom-qobject.c:27
#6 0x5555561d99f4 in object_property_set_bool qom/object.c:1188
#7 0x555555e50ae7 in qdev_device_add /root/smartx/qemu-el7/qemu-test/qdev-monitor.c:626
#8 0x555555e51213 in qmp_device_add /root/smartx/qemu-el7/qemu-test/qdev-monitor.c:806
#9 0x555555e8ff40 in hmp_device_add /root/smartx/qemu-el7/qemu-test/hmp.c:1951
#10 0x555555be889a in handle_hmp_command /root/smartx/qemu-el7/qemu-test/monitor.c:3404
#11 0x555555beac8b in monitor_command_cb /root/smartx/qemu-el7/qemu-test/monitor.c:4296
#12 0x555556433eb7 in readline_handle_byte util/readline.c:393
#13 0x555555be89ec in monitor_read /root/smartx/qemu-el7/qemu-test/monitor.c:4279
#14 0x5555563285cc in tcp_chr_read chardev/char-socket.c:470
#15 0x7ffff670b968 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4a968)
#16 0x55555640727c in glib_pollfds_poll util/main-loop.c:215
#17 0x55555640727c in os_host_main_loop_wait util/main-loop.c:238
#18 0x55555640727c in main_loop_wait util/main-loop.c:497
#19 0x555555b2d0bf in main_loop /root/smartx/qemu-el7/qemu-test/vl.c:2013
#20 0x555555b2d0bf in main /root/smartx/qemu-el7/qemu-test/vl.c:4776
#21 0x7fffdd2eb444 in __libc_start_main (/lib64/libc.so.6+0x22444)
#22 0x555555b3767a (/root/smartx/qemu-el7/qemu-test/x86_64-softmmu/qemu-system-x86_64+0x5e367a)
0x604001020d18 is located 8 bytes inside of 40-byte region [0x604001020d10,0x604001020d38)
freed by thread T0 here:
#0 0x7ffff6f00508 in __interceptor_free (/lib64/libasan.so.4+0xde508)
#1 0x7ffff671107d in g_free (/lib64/libglib-2.0.so.0+0x5007d)
previously allocated by thread T0 here:
#0 0x7ffff6f00a88 in __interceptor_calloc (/lib64/libasan.so.4+0xdea88)
#1 0x7ffff6710fc5 in g_malloc0 (/lib64/libglib-2.0.so.0+0x4ffc5)
SUMMARY: AddressSanitizer: heap-use-after-free /root/smartx/qemu-el7/qemu-test/hw/virtio/vhost.c:1473 in vhost_dev_free_inflight
Shadow bytes around the buggy address:
0x0c08801fc150: fa fa 00 00 00 00 04 fa fa fa fd fd fd fd fd fa
0x0c08801fc160: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 04 fa
0x0c08801fc170: fa fa 00 00 00 00 00 01 fa fa 00 00 00 00 04 fa
0x0c08801fc180: fa fa 00 00 00 00 00 01 fa fa 00 00 00 00 00 01
0x0c08801fc190: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 04 fa
=>0x0c08801fc1a0: fa fa fd[fd]fd fd fd fa fa fa fd fd fd fd fd fa
0x0c08801fc1b0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x0c08801fc1c0: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fd
0x0c08801fc1d0: fa fa 00 00 00 00 00 01 fa fa fd fd fd fd fd fa
0x0c08801fc1e0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fd
0x0c08801fc1f0: fa fa 00 00 00 00 00 01 fa fa fd fd fd fd fd fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==7309==ABORTING
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/block/vhost-user-blk.c | 4 ++++
hw/virtio/vhost.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 776b9af3eb..19e79b96e4 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -463,7 +463,9 @@ reconnect:
virtio_err:
g_free(s->vhost_vqs);
+ s->vhost_vqs = NULL;
g_free(s->inflight);
+ s->inflight = NULL;
for (i = 0; i < s->num_queues; i++) {
virtio_delete_queue(s->virtqs[i]);
}
@@ -484,7 +486,9 @@ static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp)
vhost_dev_cleanup(&s->dev);
vhost_dev_free_inflight(s->inflight);
g_free(s->vhost_vqs);
+ s->vhost_vqs = NULL;
g_free(s->inflight);
+ s->inflight = NULL;
for (i = 0; i < s->num_queues; i++) {
virtio_delete_queue(s->virtqs[i]);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 01ebe12f28..aff98a0ede 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1514,7 +1514,7 @@ void vhost_dev_set_config_notifier(struct vhost_dev *hdev,
void vhost_dev_free_inflight(struct vhost_inflight *inflight)
{
- if (inflight->addr) {
+ if (inflight && inflight->addr) {
qemu_memfd_free(inflight->addr, inflight->size, inflight->fd);
inflight->addr = NULL;
inflight->fd = -1;
--
2.11.0
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection
2020-04-15 3:28 [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Li Feng
2020-04-15 3:28 ` [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect Li Feng
2020-04-15 3:28 ` [PATCH 2/4] vhost-user-blk: fix invalid memory access Li Feng
@ 2020-04-15 3:28 ` Li Feng
2020-04-15 10:35 ` Marc-André Lureau
` (2 more replies)
2020-04-15 3:28 ` [PATCH 4/4] vhost-user-blk: fix crash in realize process Li Feng
2020-04-17 9:45 ` [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Michael S. Tsirkin
4 siblings, 3 replies; 32+ messages in thread
From: Li Feng @ 2020-04-15 3:28 UTC (permalink / raw)
To: kyle, Marc-André Lureau, Paolo Bonzini,
open list:All patches CC here
Cc: Li Feng
double call tcp_chr_free_connection generates a crash.
Signed-off-by: Li Feng <fengli@smartx.com>
---
chardev/char-socket.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38dda..43aab8f24b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -476,6 +476,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
SocketChardev *s = SOCKET_CHARDEV(chr);
bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
+ /* avoid re-enter when socket read/write error and disconnect event. */
+ if (s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
+ return;
+ }
+
tcp_chr_free_connection(chr);
if (s->listener) {
--
2.11.0
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/4] vhost-user-blk: fix crash in realize process
2020-04-15 3:28 [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Li Feng
` (2 preceding siblings ...)
2020-04-15 3:28 ` [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection Li Feng
@ 2020-04-15 3:28 ` Li Feng
2020-04-14 1:54 ` Raphael Norwitz
2020-04-17 9:45 ` [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Michael S. Tsirkin
4 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-04-15 3:28 UTC (permalink / raw)
To: kyle, Raphael Norwitz, Michael S. Tsirkin, Kevin Wolf, Max Reitz,
open list:Block layer core, open list:All patches CC here
Cc: Li Feng
The crash could be reproduced like this:
1. break vhost_user_write;
2. kill the vhost-user-blk target;
3. let qemu continue running;
4. start vhost-user-blk;
5. see crash!
This fix makes changes:
1. set s->connected to true after vhost_dev_init;
2. call vhost_dev_get_config when s->connected is true, otherwise the
hdev->host_ops will be nullptr.
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/block/vhost-user-blk.c | 47 +++++++++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 19e79b96e4..35386b7cb7 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -303,8 +303,6 @@ static int vhost_user_blk_connect(DeviceState *dev)
if (s->connected) {
return 0;
}
- s->connected = true;
-
s->dev.nvqs = s->num_queues;
s->dev.vqs = s->vhost_vqs;
s->dev.vq_index = 0;
@@ -318,6 +316,11 @@ static int vhost_user_blk_connect(DeviceState *dev)
strerror(-ret));
return ret;
}
+ /*
+ * set true util vhost_dev_init return ok, because CLOSE event may happen
+ * in vhost_dev_init routine.
+ */
+ s->connected = true;
/* restore vhost state */
if (virtio_device_started(vdev, vdev->status)) {
@@ -401,6 +404,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
VHostUserBlk *s = VHOST_USER_BLK(vdev);
Error *err = NULL;
int i, ret;
+ bool reconnect;
if (!s->chardev.chr) {
error_setg(errp, "vhost-user-blk: chardev is mandatory");
@@ -433,27 +437,26 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
s->inflight = g_new0(struct vhost_inflight, 1);
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
s->connected = false;
+ reconnect = false;
- qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
- NULL, (void *)dev, NULL, true);
-
-reconnect:
- if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
- error_report_err(err);
- goto virtio_err;
- }
-
- /* check whether vhost_user_blk_connect() failed or not */
- if (!s->connected) {
- goto reconnect;
- }
-
- ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
- sizeof(struct virtio_blk_config));
- if (ret < 0) {
- error_report("vhost-user-blk: get block config failed");
- goto reconnect;
- }
+ do {
+ if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
+ error_report_err(err);
+ goto virtio_err;
+ }
+ qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
+ NULL, (void *)dev, NULL, true);
+ if (s->connected) {
+ ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
+ sizeof(struct virtio_blk_config));
+ if (ret < 0) {
+ error_report("vhost-user-blk: get block config failed");
+ reconnect = true;
+ } else {
+ reconnect = false;
+ }
+ }
+ } while (!s->connected || reconnect);
if (s->blkcfg.num_queues != s->num_queues) {
s->blkcfg.num_queues = s->num_queues;
--
2.11.0
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection
2020-04-15 3:28 ` [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection Li Feng
@ 2020-04-15 10:35 ` Marc-André Lureau
2020-04-20 3:10 ` Li Feng
2020-04-28 8:51 ` [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
2020-04-28 8:54 ` [PATCH v2] " Li Feng
2 siblings, 1 reply; 32+ messages in thread
From: Marc-André Lureau @ 2020-04-15 10:35 UTC (permalink / raw)
To: Li Feng; +Cc: Paolo Bonzini, kyle, open list:All patches CC here
Hi
On Wed, Apr 15, 2020 at 5:31 AM Li Feng <fengli@smartx.com> wrote:
>
> double call tcp_chr_free_connection generates a crash.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
Could you describe how you reach the "double call".
Even better would be to write a test for it in tests/test-char.c
thanks
> ---
> chardev/char-socket.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 185fe38dda..43aab8f24b 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -476,6 +476,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
> SocketChardev *s = SOCKET_CHARDEV(chr);
> bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
>
> + /* avoid re-enter when socket read/write error and disconnect event. */
> + if (s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
> + return;
> + }
> +
> tcp_chr_free_connection(chr);
>
> if (s->listener) {
> --
> 2.11.0
>
>
> --
> The SmartX email address is only for business purpose. Any sent message
> that is not related to the business is not authorized or permitted by
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
>
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev
2020-04-15 3:28 [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Li Feng
` (3 preceding siblings ...)
2020-04-15 3:28 ` [PATCH 4/4] vhost-user-blk: fix crash in realize process Li Feng
@ 2020-04-17 9:45 ` Michael S. Tsirkin
2020-04-17 10:11 ` Li Feng
4 siblings, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2020-04-17 9:45 UTC (permalink / raw)
To: Li Feng
Cc: Kevin Wolf, open list:Block layer core,
open list:All patches CC here, Max Reitz, kyle, Paolo Bonzini,
Marc-André Lureau, Raphael Norwitz
On Wed, Apr 15, 2020 at 11:28:22AM +0800, Li Feng wrote:
> The following patches fix various crashes happened when injecting errors to
> chardev unix domain socket.
I think these are mostly unrelated fixes right?
If so pls post them separately so I know I can just apply
some and wait for others to get acked.
> The crashes are encountered when the socket is from connected to disconnected at
> vhost-user-blk realize routine.
>
> These crashes could be reproduced like this:
> 1. gdb break at vhost_user_write;
> 2. add a vhost-user-blk device through qmp;
> 3. when stop at vhost_user_write, kill the vhost-user-blk target;
> 3. let qemu continue running;
> 4. start vhost-user-blk;
> 5. see crash!
>
> The 'CLOSE' event path is core trouble maker.
>
> qemu_chr_fe_set_handlers
> -> vhost_user_blk_event(OPEN)
> -> vhost_user_blk_connect
> -> vhost_dev_init
> -> vhost_user_blk_event(CLOSE)
> -> vhost_dev_cleanup
>
>
> Li Feng (4):
> vhost-user-blk: delay vhost_user_blk_disconnect
> vhost-user-blk: fix invalid memory access
> char-socket: avoid double call tcp_chr_free_connection
> vhost-user-blk: fix crash in realize process
>
> chardev/char-socket.c | 5 ++++
> hw/block/vhost-user-blk.c | 75 ++++++++++++++++++++++++++++++++---------------
> hw/virtio/vhost.c | 2 +-
> 3 files changed, 58 insertions(+), 24 deletions(-)
>
> --
> 2.11.0
>
>
> --
> The SmartX email address is only for business purpose. Any sent message
> that is not related to the business is not authorized or permitted by
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Fwd: [PATCH 4/4] vhost-user-blk: fix crash in realize process
[not found] ` <CAHckoCwsLwqNhVHK4PF+pzt5ExkurPNsWLw7ueRG8dfOUA2rXg@mail.gmail.com>
@ 2020-04-17 10:07 ` Li Feng
0 siblings, 0 replies; 32+ messages in thread
From: Li Feng @ 2020-04-17 10:07 UTC (permalink / raw)
To: Raphael Norwitz
Cc: lifeng1519, Yang Fan, Kyle Zhang, open list:All patches CC here,
Michael S. Tsirkin
Add mail group list.
Thank you, Raphael .
Raphael Norwitz <raphael.norwitz@nutanix.com> 于2020年4月17日周五 下午12:10写道：
>
> Mostly looks good - just a few superficial notes.
>
> On Wed, Apr 15, 2020 at 11:28:26AM +0800, Li Feng wrote:
> > 1. set s->connected to true after vhost_dev_init;
> > 2. call vhost_dev_get_config when s->connected is true, otherwise the
> > hdev->host_ops will be nullptr.
>
> You mean hdev->vhost_ops, right?
>
Yes.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > hw/block/vhost-user-blk.c | 47 +++++++++++++++++++++++++----------------------
> > 1 file changed, 25 insertions(+), 22 deletions(-)
> > + /*
> > + * set true util vhost_dev_init return ok, because CLOSE event may happen
> > + * in vhost_dev_init routine.
> > + */
>
> I'm a little confused by this comment. Do you mean to say “wait until vhost_dev_init
> succeeds to set connected to true, because a close event may happen while
> vhost_dev_init is executing”?
Yes. This is the exception path:
qemu_chr_fe_set_handlers
-> vhost_user_blk_event(OPEN)
-> vhost_user_blk_connect
-> vhost_dev_init
-> vhost_user_blk_event(CLOSE)
-> vhost_dev_cleanup
We should set connected to true only when vhost_dev_init returns OK.
If a close event is triggered, we shouldn't set connected to true.
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev
2020-04-17 9:45 ` [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Michael S. Tsirkin
@ 2020-04-17 10:11 ` Li Feng
0 siblings, 0 replies; 32+ messages in thread
From: Li Feng @ 2020-04-17 10:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, lifeng1519, Yang Fan, open list:Block layer core,
open list:All patches CC here, Max Reitz, Kyle Zhang,
Paolo Bonzini, Marc-André Lureau, Raphael Norwitz
OK, I will submit this patch "vhost-user-blk: fix invalid memory
access" firstly.
This is unrelated with other three and it has been acked.
Thanks,
Feng Li
Michael S. Tsirkin <mst@redhat.com> 于2020年4月17日周五 下午5:45写道：
>
> On Wed, Apr 15, 2020 at 11:28:22AM +0800, Li Feng wrote:
> > The following patches fix various crashes happened when injecting errors to
> > chardev unix domain socket.
>
> I think these are mostly unrelated fixes right?
> If so pls post them separately so I know I can just apply
> some and wait for others to get acked.
>
>
> > The crashes are encountered when the socket is from connected to disconnected at
> > vhost-user-blk realize routine.
> >
> > These crashes could be reproduced like this:
> > 1. gdb break at vhost_user_write;
> > 2. add a vhost-user-blk device through qmp;
> > 3. when stop at vhost_user_write, kill the vhost-user-blk target;
> > 3. let qemu continue running;
> > 4. start vhost-user-blk;
> > 5. see crash!
> >
> > The 'CLOSE' event path is core trouble maker.
> >
> > qemu_chr_fe_set_handlers
> > -> vhost_user_blk_event(OPEN)
> > -> vhost_user_blk_connect
> > -> vhost_dev_init
> > -> vhost_user_blk_event(CLOSE)
> > -> vhost_dev_cleanup
> >
> >
> > Li Feng (4):
> > vhost-user-blk: delay vhost_user_blk_disconnect
> > vhost-user-blk: fix invalid memory access
> > char-socket: avoid double call tcp_chr_free_connection
> > vhost-user-blk: fix crash in realize process
> >
> > chardev/char-socket.c | 5 ++++
> > hw/block/vhost-user-blk.c | 75 ++++++++++++++++++++++++++++++++---------------
> > hw/virtio/vhost.c | 2 +-
> > 3 files changed, 58 insertions(+), 24 deletions(-)
> >
> > --
> > 2.11.0
> >
> >
> > --
> > The SmartX email address is only for business purpose. Any sent message
> > that is not related to the business is not authorized or permitted by
> > SmartX.
> > 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> >
>
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection
2020-04-15 10:35 ` Marc-André Lureau
@ 2020-04-20 3:10 ` Li Feng
2020-04-20 14:28 ` Marc-André Lureau
0 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-04-20 3:10 UTC (permalink / raw)
To: Marc-André Lureau
Cc: lifeng1519, Paolo Bonzini, Yang Fan, Kyle Zhang,
open list:All patches CC here
Hi Lureau,
Thanks for your comment.
I have spent some time to reproduce this crash, so that delay my
reply, apologies.
This is the description of this bug using gdb:
1. Set break points using gdb;
b chardev/char-socket.c:410 if s->state == TCP_CHARDEV_STATE_DISCONNECTED
b break vhost_user_write
2. hotplug a vhost-blk device:
echo "chardev-add
socket,id=spdk_vhost_blk2,path=/vhost-blk.0,reconnect=1 "| nc -U
/tmp/a.socket
echo "device_add
vhost-user-blk-pci,chardev=spdk_vhost_blk2,id=spdk_vhost_blk2,num-queues=4"
| nc -U /tmp/a.socket
3. Gdb will stop at vhost_user_write, and CTRL-C the vhost target.
4. Put 'c' to let gdb continue.
You will see a crash:
192 login: qemu-system-x86_64: chardev/char-socket.c:125:
qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed.
The related code:
394 static void tcp_chr_free_connection(Chardev *chr)
395 {
396 SocketChardev *s = SOCKET_CHARDEV(chr);
397 int i;
398
399 if (s->read_msgfds_num) {
400 for (i = 0; i < s->read_msgfds_num; i++) {
401 close(s->read_msgfds[i]);
402 }
403 g_free(s->read_msgfds);
404 s->read_msgfds = NULL;
405 s->read_msgfds_num = 0;
406 }
407
408 remove_hup_source(s);
409
410 tcp_set_msgfds(chr, NULL, 0);
411 remove_fd_in_watch(chr);
412 object_unref(OBJECT(s->sioc));
413 s->sioc = NULL;
414 object_unref(OBJECT(s->ioc));
415 s->ioc = NULL;
416 g_free(chr->filename);
417 chr->filename = NULL;
418 tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
419 }
#0 0x0000555555c1aae8 in tcp_chr_free_connection
(chr=chr@entry=0x55555751ee00) at chardev/char-socket.c:410
#1 0x0000555555c1aec8 in tcp_chr_disconnect_locked
(chr=0x55555751ee00) at chardev/char-socket.c:479
#2 0x0000555555c1af5d in tcp_chr_disconnect (chr=0x55555751ee00) at
chardev/char-socket.c:497
#3 0x0000555555c16715 in qemu_chr_fe_set_handlers
(b=b@entry=0x5555575f3b48, fd_can_read=fd_can_read@entry=0x0,
fd_read=fd_
read@entry=0x0, fd_event=fd_event@entry=0x55555588e740
<vhost_user_blk_event>, be_change=be_change@entry=0x0, opaque=opaque@
entry=0x5555575f3990, context=0x0, set_open=true) at chardev/char-fe.c:304
#4 0x000055555588e5c0 in vhost_user_blk_device_realize
(dev=0x5555575f3990, errp=<optimized out>)
at /root/qemu-master/hw/block/vhost-user-blk.c:447
#5 0x00005555558ca478 in virtio_device_realize (dev=0x5555575f3990,
errp=0x7fffffffbb90)
at /root/qemu-master/hw/virtio/virtio.c:3615
#6 0x00005555559dc842 in device_set_realized (obj=<optimized out>,
value=<optimized out>, errp=0x7fffffffbd20)
at hw/core/qdev.c:891
#7 0x0000555555b78e07 in property_set_bool (obj=0x5555575f3990,
v=<optimized out>, name=<optimized out>, opaque=0x555556453
040, errp=0x7fffffffbd20) at qom/object.c:2238
#8 0x0000555555b7db3f in object_property_set_qobject
(obj=obj@entry=0x5555575f3990, value=value@entry=0x555556ab89c0, name=
name@entry=0x555555d15308 "realized", errp=errp@entry=0x7fffffffbd20)
at qom/qom-qobject.c:26
#9 0x0000555555b7b2c5 in object_property_set_bool
(obj=0x5555575f3990, value=<optimized out>, name=0x555555d15308
"realized
", errp=0x7fffffffbd20) at qom/object.c:1390
#10 0x0000555555af13b2 in virtio_pci_realize (pci_dev=0x5555575eb800,
errp=0x7fffffffbd20) at hw/virtio/virtio-pci.c:1807
#11 0x0000555555a7a14b in pci_qdev_realize (qdev=<optimized out>,
errp=<optimized out>) at hw/pci/pci.c:2098
#12 0x00005555559dc842 in device_set_realized (obj=<optimized out>,
value=<optimized out>, errp=0x7fffffffbef8)
at hw/core/qdev.c:891
(gdb) p s
$23 = (SocketChardev *) 0x55555751ee00
(gdb) p s->state
$24 = TCP_CHARDEV_STATE_DISCONNECTED
At 410 line of char-socket.c, the state has been changed to
TCP_CHARDEV_STATE_DISCONNECTED before tcp_chr_change_state(s,
TCP_CHARDEV_STATE_DISCONNECTED);
It means the tcp_chr_disconnect_locked is called by more than one
times, and the tcp socket has been freed before.
The crash reason is s->reconnect_timer is set in the previous call of
tcp_chr_disconnect_locked.
The another approach fix maybe like this:
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38dda..94957de367 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);
}
}
The base code is here:
474 static void tcp_chr_disconnect_locked(Chardev *chr)
475 {
476 SocketChardev *s = SOCKET_CHARDEV(chr);
477 bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
478
479 tcp_chr_free_connection(chr);
480
481 if (s->listener) {
482 qio_net_listener_set_client_func_full(s->listener, tcp_chr_accept,
483 chr, NULL, chr->gcontext);
484 }
485 update_disconnected_filename(s);
486 if (emit_close) {
487 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
488 }
489 if (s->reconnect_time) {
490 qemu_chr_socket_restart_timer(chr);
491 }
492 }
Which one is better?
I don't know how to inject an error at socket write when writing tests
code(tests/test-char.c ).
Any tips?
Thanks,
Feng Li
Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年4月15日周三 下午6:35写道：
>
> Hi
>
> On Wed, Apr 15, 2020 at 5:31 AM Li Feng <fengli@smartx.com> wrote:
> >
> > double call tcp_chr_free_connection generates a crash.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
>
> Could you describe how you reach the "double call".
>
> Even better would be to write a test for it in tests/test-char.c
>
> thanks
>
> > ---
> > chardev/char-socket.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 185fe38dda..43aab8f24b 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -476,6 +476,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
> > SocketChardev *s = SOCKET_CHARDEV(chr);
> > bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> >
> > + /* avoid re-enter when socket read/write error and disconnect event. */
> > + if (s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
> > + return;
> > + }
> > +
> > tcp_chr_free_connection(chr);
> >
> > if (s->listener) {
> > --
> > 2.11.0
> >
> >
> > --
> > The SmartX email address is only for business purpose. Any sent message
> > that is not related to the business is not authorized or permitted by
> > SmartX.
> > 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> >
> >
> >
>
>
> --
> Marc-André Lureau
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect
2020-04-14 1:24 ` Raphael Norwitz
@ 2020-04-20 11:18 ` Li Feng
0 siblings, 0 replies; 32+ messages in thread
From: Li Feng @ 2020-04-20 11:18 UTC (permalink / raw)
To: Raphael Norwitz
Cc: lifeng1519, Yang Fan, Kyle Zhang, open list:All patches CC here,
Michael S. Tsirkin
Hi Norwitz ，
Thanks for your good suggestion.
I got this fix from net/vhost-user.c, it has the same issue with this case.
Your suggestion is a good option.
I'm trying to do some work. but there is another crash issue ...
I need some time to make your idea be workable.
This is the net/vhost-user patch:
commit e7c83a885f865128ae3cf1946f8cb538b63cbfba
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date: Mon Feb 27 14:49:56 2017 +0400
vhost-user: delay vhost_user_stop
Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
may trigger a disconnect events, calling vhost_user_stop() and clearing
all the vhost_dev strutures holding data that vhost.c functions expect
to remain valid. Delay the cleanup to keep the vhost_dev structure
valid during the vhost.c functions.
Feng Li
Raphael Norwitz <raphael.norwitz@nutanix.com> 于2020年4月17日周五 上午11:41写道：
>
> On Wed, Apr 15, 2020 at 11:28:23AM +0800, Li Feng wrote:
> >
> > switch (event) {
> > case CHR_EVENT_OPENED:
> > @@ -363,7 +376,16 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> > }
> > break;
> > case CHR_EVENT_CLOSED:
> > - vhost_user_blk_disconnect(dev);
> > + /*
> > + * a close event may happen during a read/write, but vhost
> > + * code assumes the vhost_dev remains setup, so delay the
> > + * stop & clear to idle.
> > + */
> > + ctx = qemu_get_current_aio_context();
> > +
> > + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL,
> > + NULL, NULL, NULL, false);
> > + aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
>
> This seems a bit racy. What’s to stop the async operation from executing before
> the next read?
>
> If the issue is just that the vhost_dev state is being destroyed too early, why
> don’t we rather move the vhost_dev_cleanup() call from vhost_user_blk_disconnect()
> to vhost_user_blk_connect()? We may need to add some state to tell if this is the
> first connect or a reconnect so we don’t call vhost_dev_cleanup() on initial
> connect, but as long as we call vhost_dev_cleanup() before vhost_dev_init()
> every time the device reconnects it shouldn’t matter that we keep that state
> around.
>
> Thoughts?
>
> > break;
> > case CHR_EVENT_BREAK:
> > case CHR_EVENT_MUX_IN:
--
The SmartX email address is only for business purpose. Any sent message
that is not related to the business is not authorized or permitted by
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection
2020-04-20 3:10 ` Li Feng
@ 2020-04-20 14:28 ` Marc-André Lureau
0 siblings, 0 replies; 32+ messages in thread
From: Marc-André Lureau @ 2020-04-20 14:28 UTC (permalink / raw)
To: Li Feng
Cc: Li Feng, Paolo Bonzini, Yang Fan, Kyle Zhang,
open list:All patches CC here
Hi
On Mon, Apr 20, 2020 at 5:10 AM Li Feng <fengli@smartx.com> wrote:
>
> Hi Lureau,
>
> Thanks for your comment.
> I have spent some time to reproduce this crash, so that delay my
> reply, apologies.
>
> This is the description of this bug using gdb:
> 1. Set break points using gdb;
> b chardev/char-socket.c:410 if s->state == TCP_CHARDEV_STATE_DISCONNECTED
> b break vhost_user_write
>
> 2. hotplug a vhost-blk device:
> echo "chardev-add
> socket,id=spdk_vhost_blk2,path=/vhost-blk.0,reconnect=1 "| nc -U
> /tmp/a.socket
> echo "device_add
> vhost-user-blk-pci,chardev=spdk_vhost_blk2,id=spdk_vhost_blk2,num-queues=4"
> | nc -U /tmp/a.socket
>
> 3. Gdb will stop at vhost_user_write, and CTRL-C the vhost target.
> 4. Put 'c' to let gdb continue.
> You will see a crash:
>
> 192 login: qemu-system-x86_64: chardev/char-socket.c:125:
> qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed.
>
> The related code:
> 394 static void tcp_chr_free_connection(Chardev *chr)
> 395 {
> 396 SocketChardev *s = SOCKET_CHARDEV(chr);
> 397 int i;
> 398
> 399 if (s->read_msgfds_num) {
> 400 for (i = 0; i < s->read_msgfds_num; i++) {
> 401 close(s->read_msgfds[i]);
> 402 }
> 403 g_free(s->read_msgfds);
> 404 s->read_msgfds = NULL;
> 405 s->read_msgfds_num = 0;
> 406 }
> 407
> 408 remove_hup_source(s);
> 409
> 410 tcp_set_msgfds(chr, NULL, 0);
> 411 remove_fd_in_watch(chr);
> 412 object_unref(OBJECT(s->sioc));
> 413 s->sioc = NULL;
> 414 object_unref(OBJECT(s->ioc));
> 415 s->ioc = NULL;
> 416 g_free(chr->filename);
> 417 chr->filename = NULL;
> 418 tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
> 419 }
>
> #0 0x0000555555c1aae8 in tcp_chr_free_connection
> (chr=chr@entry=0x55555751ee00) at chardev/char-socket.c:410
> #1 0x0000555555c1aec8 in tcp_chr_disconnect_locked
> (chr=0x55555751ee00) at chardev/char-socket.c:479
> #2 0x0000555555c1af5d in tcp_chr_disconnect (chr=0x55555751ee00) at
> chardev/char-socket.c:497
> #3 0x0000555555c16715 in qemu_chr_fe_set_handlers
> (b=b@entry=0x5555575f3b48, fd_can_read=fd_can_read@entry=0x0,
> fd_read=fd_
> read@entry=0x0, fd_event=fd_event@entry=0x55555588e740
> <vhost_user_blk_event>, be_change=be_change@entry=0x0, opaque=opaque@
> entry=0x5555575f3990, context=0x0, set_open=true) at chardev/char-fe.c:304
> #4 0x000055555588e5c0 in vhost_user_blk_device_realize
> (dev=0x5555575f3990, errp=<optimized out>)
> at /root/qemu-master/hw/block/vhost-user-blk.c:447
> #5 0x00005555558ca478 in virtio_device_realize (dev=0x5555575f3990,
> errp=0x7fffffffbb90)
> at /root/qemu-master/hw/virtio/virtio.c:3615
> #6 0x00005555559dc842 in device_set_realized (obj=<optimized out>,
> value=<optimized out>, errp=0x7fffffffbd20)
> at hw/core/qdev.c:891
> #7 0x0000555555b78e07 in property_set_bool (obj=0x5555575f3990,
> v=<optimized out>, name=<optimized out>, opaque=0x555556453
> 040, errp=0x7fffffffbd20) at qom/object.c:2238
> #8 0x0000555555b7db3f in object_property_set_qobject
> (obj=obj@entry=0x5555575f3990, value=value@entry=0x555556ab89c0, name=
> name@entry=0x555555d15308 "realized", errp=errp@entry=0x7fffffffbd20)
> at qom/qom-qobject.c:26
> #9 0x0000555555b7b2c5 in object_property_set_bool
> (obj=0x5555575f3990, value=<optimized out>, name=0x555555d15308
> "realized
> ", errp=0x7fffffffbd20) at qom/object.c:1390
> #10 0x0000555555af13b2 in virtio_pci_realize (pci_dev=0x5555575eb800,
> errp=0x7fffffffbd20) at hw/virtio/virtio-pci.c:1807
> #11 0x0000555555a7a14b in pci_qdev_realize (qdev=<optimized out>,
> errp=<optimized out>) at hw/pci/pci.c:2098
> #12 0x00005555559dc842 in device_set_realized (obj=<optimized out>,
> value=<optimized out>, errp=0x7fffffffbef8)
> at hw/core/qdev.c:891
>
> (gdb) p s
> $23 = (SocketChardev *) 0x55555751ee00
> (gdb) p s->state
> $24 = TCP_CHARDEV_STATE_DISCONNECTED
>
> At 410 line of char-socket.c, the state has been changed to
> TCP_CHARDEV_STATE_DISCONNECTED before tcp_chr_change_state(s,
> TCP_CHARDEV_STATE_DISCONNECTED);
> It means the tcp_chr_disconnect_locked is called by more than one
> times, and the tcp socket has been freed before.
>
> The crash reason is s->reconnect_timer is set in the previous call of
> tcp_chr_disconnect_locked.
> The another approach fix maybe like this:
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 185fe38dda..94957de367 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);
> }
> }
>
> The base code is here:
> 474 static void tcp_chr_disconnect_locked(Chardev *chr)
> 475 {
> 476 SocketChardev *s = SOCKET_CHARDEV(chr);
> 477 bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> 478
> 479 tcp_chr_free_connection(chr);
> 480
> 481 if (s->listener) {
> 482 qio_net_listener_set_client_func_full(s->listener, tcp_chr_accept,
> 483 chr, NULL, chr->gcontext);
> 484 }
> 485 update_disconnected_filename(s);
> 486 if (emit_close) {
> 487 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> 488 }
> 489 if (s->reconnect_time) {
> 490 qemu_chr_socket_restart_timer(chr);
> 491 }
> 492 }
>
> Which one is better?
+ if (s->reconnect_time && !s->reconnect_timer)
Looks like a good solution to me.
>
> I don't know how to inject an error at socket write when writing tests
> code(tests/test-char.c ).
> Any tips?
You could check the patch I just sent fixing a related issue:
https://patchew.org/QEMU/20200420112012.567284-1-marcandre.lureau@redhat.com/
In your case, it might be as easy as calling qemu_chr_fe_disconnect()
2 times on a reconnect socket, I'll let you figure out.
Thanks!
>
> Thanks,
> Feng Li
>
> Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年4月15日周三 下午6:35写道：
>
> >
> > Hi
> >
> > On Wed, Apr 15, 2020 at 5:31 AM Li Feng <fengli@smartx.com> wrote:
> > >
> > > double call tcp_chr_free_connection generates a crash.
> > >
> > > Signed-off-by: Li Feng <fengli@smartx.com>
> >
> > Could you describe how you reach the "double call".
> >
> > Even better would be to write a test for it in tests/test-char.c
> >
> > thanks
> >
> > > ---
> > > chardev/char-socket.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 185fe38dda..43aab8f24b 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -476,6 +476,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
> > > SocketChardev *s = SOCKET_CHARDEV(chr);
> > > bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> > >
> > > + /* avoid re-enter when socket read/write error and disconnect event. */
> > > + if (s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
> > > + return;
> > > + }
> > > +
> > > tcp_chr_free_connection(chr);
> > >
> > > if (s->listener) {
> > > --
> > > 2.11.0
> > >
> > >
> > > --
> > > The SmartX email address is only for business purpose. Any sent message
> > > that is not related to the business is not authorized or permitted by
> > > SmartX.
> > > 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> > >
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
>
> --
> The SmartX email address is only for business purpose. Any sent message
> that is not related to the business is not authorized or permitted by
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-04-15 3:28 ` [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection Li Feng
2020-04-15 10:35 ` Marc-André Lureau
@ 2020-04-28 8:51 ` Li Feng
[not found] ` <20200508051441.8143-1-fengli@smartx.com>
2020-05-11 12:39 ` [PATCH v4] " Li Feng
2020-04-28 8:54 ` [PATCH v2] " Li Feng
2 siblings, 2 replies; 32+ messages in thread
From: Li Feng @ 2020-04-28 8:51 UTC (permalink / raw)
To: kyle, lifeng1519, dimastep, Marc-André 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>
---
v2:
- Rewrite the solution.
- Add test to reproduce this issue.
chardev/char-socket.c | 2 +-
tests/test-char.c | 48 ++++++++++++++++++++++++++++++++++++++----------
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..13dbbfe2a3 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,23 @@ char_socket_event(void *opaque, QEMUChrEvent event)
data->event = event;
}
+static void
+char_socket_event_with_error(void *opaque, QEMUChrEvent event)
+{
+ CharSocketTestData *data = opaque;
+ CharBackend *be = data->be;
+ data->event = event;
+ switch (event) {
+ case CHR_EVENT_OPENED:
+ 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)
@@ -783,6 +802,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);
@@ -869,6 +889,7 @@ typedef struct {
const char *reconnect;
bool wait_connected;
bool fd_pass;
+ char_socket_cb event_cb;
} CharSocketClientTestConfig;
static void char_socket_client_dupid_test(gconstpointer opaque)
@@ -920,6 +941,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
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;
@@ -983,8 +1005,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);
@@ -1022,7 +1045,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;
@@ -1467,19 +1490,22 @@ 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", false, false }; \
+ { addr, ",reconnect=1", false, false, char_socket_event }; \
+ static CharSocketClientTestConfig client8 ## 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, \
@@ -1493,7 +1519,9 @@ int main(int argc, char **argv)
g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
&client6 ##name, char_socket_client_test); \
g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
- &client7 ##name, char_socket_client_dupid_test)
+ &client7 ##name, char_socket_client_dupid_test);\
+ g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
+ &client8 ##name, char_socket_client_test)
if (has_ipv4) {
SOCKET_SERVER_TEST(tcp, &tcpaddr);
--
2.11.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-04-15 3:28 ` [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection Li Feng
2020-04-15 10:35 ` Marc-André Lureau
2020-04-28 8:51 ` [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
@ 2020-04-28 8:54 ` Li Feng
2020-04-28 8:59 ` Li Feng
2 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-04-28 8:54 UTC (permalink / raw)
To: kyle, lifeng1519, dimastep, Marc-André 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>
---
v2:
- Rewrite the solution.
- Add test to reproduce this issue.
chardev/char-socket.c | 2 +-
tests/test-char.c | 48 ++++++++++++++++++++++++++++++++++++++----------
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..13dbbfe2a3 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,23 @@ char_socket_event(void *opaque, QEMUChrEvent event)
data->event = event;
}
+static void
+char_socket_event_with_error(void *opaque, QEMUChrEvent event)
+{
+ CharSocketTestData *data = opaque;
+ CharBackend *be = data->be;
+ data->event = event;
+ switch (event) {
+ case CHR_EVENT_OPENED:
+ 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)
@@ -783,6 +802,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);
@@ -869,6 +889,7 @@ typedef struct {
const char *reconnect;
bool wait_connected;
bool fd_pass;
+ char_socket_cb event_cb;
} CharSocketClientTestConfig;
static void char_socket_client_dupid_test(gconstpointer opaque)
@@ -920,6 +941,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
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;
@@ -983,8 +1005,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);
@@ -1022,7 +1045,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;
@@ -1467,19 +1490,22 @@ 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", false, false }; \
+ { addr, ",reconnect=1", false, false, char_socket_event }; \
+ static CharSocketClientTestConfig client8 ## 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, \
@@ -1493,7 +1519,9 @@ int main(int argc, char **argv)
g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
&client6 ##name, char_socket_client_test); \
g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
- &client7 ##name, char_socket_client_dupid_test)
+ &client7 ##name, char_socket_client_dupid_test);\
+ g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
+ &client8 ##name, char_socket_client_test)
if (has_ipv4) {
SOCKET_SERVER_TEST(tcp, &tcpaddr);
--
2.11.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-04-28 8:54 ` [PATCH v2] " Li Feng
@ 2020-04-28 8:59 ` Li Feng
2020-04-28 11:05 ` Marc-André Lureau
0 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-04-28 8:59 UTC (permalink / raw)
To: Kyle Zhang, Feng Li, Dima Stepanov, Marc-André Lureau,
Paolo Bonzini, open list:All patches CC here, Yutian Zhou
Sorry for sending the weird same mail out.
Ignore the second one.
Hi Lureau,
I found another issue when running my new test: tests/test-char -p
/char/socket/client/reconnect-error/unix
The backtrace like this:
#0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
#1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
#2 0x00005555555aaa34 in error_handle_fatal (errp=<optimized out>,
err=0x7fffec0012d0) at util/error.c:40
#3 0x00005555555aab0d in error_setv (errp=0x555555802a08
<error_abort>, src=0x5555555c4220 "io/channel.c", line=148,
func=0x5555555c4520 <__func__.17450> "qio_channel_readv_all",
err_class=ERROR_CLASS_GENERIC_ERROR, fmt=<optimized out>,
ap=0x7ffff423bb10, suffix=0x0) at util/error.c:73
#4 0x00005555555aac90 in error_setg_internal
(errp=errp@entry=0x555555802a08 <error_abort>,
src=src@entry=0x5555555c4220 "io/channel.c", line=line@entry=148,
func=func@entry=0x5555555c4520 <__func__.17450>
"qio_channel_readv_all", fmt=fmt@entry=0x5555555c4340 "Unexpected
end-of-file before all bytes were read") at util/error.c:97
#5 0x000055555556c1fc in qio_channel_readv_all (ioc=<optimized out>,
iov=<optimized out>, niov=<optimized out>, errp=0x555555802a08
<error_abort>) at io/channel.c:147
#6 0x000055555556c23a in qio_channel_read_all (ioc=<optimized out>,
buf=<optimized out>, buflen=<optimized out>, errp=<optimized out>) at
io/channel.c:247
#7 0x000055555556ad0e in char_socket_ping_pong (ioc=0x7fffec0008c0)
at tests/test-char.c:727
#8 0x000055555556adcf in char_socket_client_server_thread
(data=data@entry=0x55555582e350) at tests/test-char.c:881
#9 0x00005555555a9556 in qemu_thread_start (args=<optimized out>) at
util/qemu-thread-posix.c:519
#10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
#11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
I think this is a new issue of qemu, not my test issue.
How do you think?
Thanks,
Feng Li
Li Feng <fengli@smartx.com> 于2020年4月28日周二 下午4:53写道：
>
> 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>
> ---
> v2:
> - Rewrite the solution.
> - Add test to reproduce this issue.
>
> chardev/char-socket.c | 2 +-
> tests/test-char.c | 48 ++++++++++++++++++++++++++++++++++++++----------
> 2 files changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..13dbbfe2a3 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,23 @@ char_socket_event(void *opaque, QEMUChrEvent event)
> data->event = event;
> }
>
> +static void
> +char_socket_event_with_error(void *opaque, QEMUChrEvent event)
> +{
> + CharSocketTestData *data = opaque;
> + CharBackend *be = data->be;
> + data->event = event;
> + switch (event) {
> + case CHR_EVENT_OPENED:
> + 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)
> @@ -783,6 +802,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);
> @@ -869,6 +889,7 @@ typedef struct {
> const char *reconnect;
> bool wait_connected;
> bool fd_pass;
> + char_socket_cb event_cb;
> } CharSocketClientTestConfig;
>
> static void char_socket_client_dupid_test(gconstpointer opaque)
> @@ -920,6 +941,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> 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;
> @@ -983,8 +1005,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);
> @@ -1022,7 +1045,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;
> @@ -1467,19 +1490,22 @@ 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", false, false }; \
> + { addr, ",reconnect=1", false, false, char_socket_event }; \
> + static CharSocketClientTestConfig client8 ## 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, \
> @@ -1493,7 +1519,9 @@ int main(int argc, char **argv)
> g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> &client6 ##name, char_socket_client_test); \
> g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> - &client7 ##name, char_socket_client_dupid_test)
> + &client7 ##name, char_socket_client_dupid_test);\
> + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> + &client8 ##name, char_socket_client_test)
>
> if (has_ipv4) {
> SOCKET_SERVER_TEST(tcp, &tcpaddr);
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-04-28 8:59 ` Li Feng
@ 2020-04-28 11:05 ` Marc-André Lureau
2020-04-28 14:09 ` Feng Li
0 siblings, 1 reply; 32+ messages in thread
From: Marc-André Lureau @ 2020-04-28 11:05 UTC (permalink / raw)
To: Li Feng
Cc: Feng Li, open list:All patches CC here, Yutian Zhou, Kyle Zhang,
Paolo Bonzini, Dima Stepanov
Hi
On Tue, Apr 28, 2020 at 10:59 AM Li Feng <fengli@smartx.com> wrote:
>
> Sorry for sending the weird same mail out.
> Ignore the second one.
>
> Hi Lureau,
>
> I found another issue when running my new test: tests/test-char -p
> /char/socket/client/reconnect-error/unix
> The backtrace like this:
> #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
> #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
> #2 0x00005555555aaa34 in error_handle_fatal (errp=<optimized out>,
> err=0x7fffec0012d0) at util/error.c:40
> #3 0x00005555555aab0d in error_setv (errp=0x555555802a08
> <error_abort>, src=0x5555555c4220 "io/channel.c", line=148,
> func=0x5555555c4520 <__func__.17450> "qio_channel_readv_all",
> err_class=ERROR_CLASS_GENERIC_ERROR, fmt=<optimized out>,
> ap=0x7ffff423bb10, suffix=0x0) at util/error.c:73
> #4 0x00005555555aac90 in error_setg_internal
> (errp=errp@entry=0x555555802a08 <error_abort>,
> src=src@entry=0x5555555c4220 "io/channel.c", line=line@entry=148,
> func=func@entry=0x5555555c4520 <__func__.17450>
> "qio_channel_readv_all", fmt=fmt@entry=0x5555555c4340 "Unexpected
> end-of-file before all bytes were read") at util/error.c:97
> #5 0x000055555556c1fc in qio_channel_readv_all (ioc=<optimized out>,
> iov=<optimized out>, niov=<optimized out>, errp=0x555555802a08
> <error_abort>) at io/channel.c:147
> #6 0x000055555556c23a in qio_channel_read_all (ioc=<optimized out>,
> buf=<optimized out>, buflen=<optimized out>, errp=<optimized out>) at
> io/channel.c:247
> #7 0x000055555556ad0e in char_socket_ping_pong (ioc=0x7fffec0008c0)
> at tests/test-char.c:727
> #8 0x000055555556adcf in char_socket_client_server_thread
> (data=data@entry=0x55555582e350) at tests/test-char.c:881
> #9 0x00005555555a9556 in qemu_thread_start (args=<optimized out>) at
> util/qemu-thread-posix.c:519
> #10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
> #11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
>
> I think this is a new issue of qemu, not my test issue.
> How do you think?
No idea, it could be a bug in the test itself. How did you produce it?
>
> Thanks,
>
> Feng Li
>
> Li Feng <fengli@smartx.com> 于2020年4月28日周二 下午4:53写道：
>
> >
> > 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>
> > ---
> > v2:
> > - Rewrite the solution.
> > - Add test to reproduce this issue.
> >
> > chardev/char-socket.c | 2 +-
> > tests/test-char.c | 48 ++++++++++++++++++++++++++++++++++++++----------
> > 2 files changed, 39 insertions(+), 11 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..13dbbfe2a3 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,23 @@ char_socket_event(void *opaque, QEMUChrEvent event)
> > data->event = event;
> > }
> >
> > +static void
> > +char_socket_event_with_error(void *opaque, QEMUChrEvent event)
> > +{
> > + CharSocketTestData *data = opaque;
> > + CharBackend *be = data->be;
> > + data->event = event;
> > + switch (event) {
> > + case CHR_EVENT_OPENED:
> > + 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)
> > @@ -783,6 +802,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);
> > @@ -869,6 +889,7 @@ typedef struct {
> > const char *reconnect;
> > bool wait_connected;
> > bool fd_pass;
> > + char_socket_cb event_cb;
> > } CharSocketClientTestConfig;
> >
> > static void char_socket_client_dupid_test(gconstpointer opaque)
> > @@ -920,6 +941,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> > 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;
> > @@ -983,8 +1005,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);
> > @@ -1022,7 +1045,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;
> > @@ -1467,19 +1490,22 @@ 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", false, false }; \
> > + { addr, ",reconnect=1", false, false, char_socket_event }; \
> > + static CharSocketClientTestConfig client8 ## 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, \
> > @@ -1493,7 +1519,9 @@ int main(int argc, char **argv)
> > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> > &client6 ##name, char_socket_client_test); \
> > g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> > - &client7 ##name, char_socket_client_dupid_test)
> > + &client7 ##name, char_socket_client_dupid_test);\
> > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> > + &client8 ##name, char_socket_client_test)
> >
> > if (has_ipv4) {
> > SOCKET_SERVER_TEST(tcp, &tcpaddr);
> > --
> > 2.11.0
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-04-28 11:05 ` Marc-André Lureau
@ 2020-04-28 14:09 ` Feng Li
0 siblings, 0 replies; 32+ messages in thread
From: Feng Li @ 2020-04-28 14:09 UTC (permalink / raw)
To: Marc-André Lureau
Cc: open list:All patches CC here, Yutian Zhou, Kyle Zhang,
Paolo Bonzini, Li Feng, Dima Stepanov
Apply the above patch, then `make check-unit`
or
`tests/test-char -p /char/socket/client/reconnect-error/unix`
Marc-André Lureau <marcandre.lureau@redhat.com> 于2020年4月28日周二 下午7:06写道：
>
> Hi
>
> On Tue, Apr 28, 2020 at 10:59 AM Li Feng <fengli@smartx.com> wrote:
> >
> > Sorry for sending the weird same mail out.
> > Ignore the second one.
> >
> > Hi Lureau,
> >
> > I found another issue when running my new test: tests/test-char -p
> > /char/socket/client/reconnect-error/unix
> > The backtrace like this:
> > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
> > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
> > #2 0x00005555555aaa34 in error_handle_fatal (errp=<optimized out>,
> > err=0x7fffec0012d0) at util/error.c:40
> > #3 0x00005555555aab0d in error_setv (errp=0x555555802a08
> > <error_abort>, src=0x5555555c4220 "io/channel.c", line=148,
> > func=0x5555555c4520 <__func__.17450> "qio_channel_readv_all",
> > err_class=ERROR_CLASS_GENERIC_ERROR, fmt=<optimized out>,
> > ap=0x7ffff423bb10, suffix=0x0) at util/error.c:73
> > #4 0x00005555555aac90 in error_setg_internal
> > (errp=errp@entry=0x555555802a08 <error_abort>,
> > src=src@entry=0x5555555c4220 "io/channel.c", line=line@entry=148,
> > func=func@entry=0x5555555c4520 <__func__.17450>
> > "qio_channel_readv_all", fmt=fmt@entry=0x5555555c4340 "Unexpected
> > end-of-file before all bytes were read") at util/error.c:97
> > #5 0x000055555556c1fc in qio_channel_readv_all (ioc=<optimized out>,
> > iov=<optimized out>, niov=<optimized out>, errp=0x555555802a08
> > <error_abort>) at io/channel.c:147
> > #6 0x000055555556c23a in qio_channel_read_all (ioc=<optimized out>,
> > buf=<optimized out>, buflen=<optimized out>, errp=<optimized out>) at
> > io/channel.c:247
> > #7 0x000055555556ad0e in char_socket_ping_pong (ioc=0x7fffec0008c0)
> > at tests/test-char.c:727
> > #8 0x000055555556adcf in char_socket_client_server_thread
> > (data=data@entry=0x55555582e350) at tests/test-char.c:881
> > #9 0x00005555555a9556 in qemu_thread_start (args=<optimized out>) at
> > util/qemu-thread-posix.c:519
> > #10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
> > #11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
> >
> > I think this is a new issue of qemu, not my test issue.
> > How do you think?
>
>
> No idea, it could be a bug in the test itself. How did you produce it?
>
> >
> > Thanks,
> >
> > Feng Li
> >
> > Li Feng <fengli@smartx.com> 于2020年4月28日周二 下午4:53写道：
> >
> > >
> > > 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>
> > > ---
> > > v2:
> > > - Rewrite the solution.
> > > - Add test to reproduce this issue.
> > >
> > > chardev/char-socket.c | 2 +-
> > > tests/test-char.c | 48 ++++++++++++++++++++++++++++++++++++++----------
> > > 2 files changed, 39 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..13dbbfe2a3 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,23 @@ char_socket_event(void *opaque, QEMUChrEvent event)
> > > data->event = event;
> > > }
> > >
> > > +static void
> > > +char_socket_event_with_error(void *opaque, QEMUChrEvent event)
> > > +{
> > > + CharSocketTestData *data = opaque;
> > > + CharBackend *be = data->be;
> > > + data->event = event;
> > > + switch (event) {
> > > + case CHR_EVENT_OPENED:
> > > + 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)
> > > @@ -783,6 +802,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);
> > > @@ -869,6 +889,7 @@ typedef struct {
> > > const char *reconnect;
> > > bool wait_connected;
> > > bool fd_pass;
> > > + char_socket_cb event_cb;
> > > } CharSocketClientTestConfig;
> > >
> > > static void char_socket_client_dupid_test(gconstpointer opaque)
> > > @@ -920,6 +941,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> > > 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;
> > > @@ -983,8 +1005,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);
> > > @@ -1022,7 +1045,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;
> > > @@ -1467,19 +1490,22 @@ 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", false, false }; \
> > > + { addr, ",reconnect=1", false, false, char_socket_event }; \
> > > + static CharSocketClientTestConfig client8 ## 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, \
> > > @@ -1493,7 +1519,9 @@ int main(int argc, char **argv)
> > > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> > > &client6 ##name, char_socket_client_test); \
> > > g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> > > - &client7 ##name, char_socket_client_dupid_test)
> > > + &client7 ##name, char_socket_client_dupid_test);\
> > > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> > > + &client8 ##name, char_socket_client_test)
> > >
> > > if (has_ipv4) {
> > > SOCKET_SERVER_TEST(tcp, &tcpaddr);
> > > --
> > > 2.11.0
> > >
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0
[not found] ` <20200508051441.8143-1-fengli@smartx.com>
@ 2020-05-08 5:14 ` Li Feng
2020-05-08 12:32 ` Marc-André Lureau
2020-05-08 5:14 ` [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
1 sibling, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-05-08 5:14 UTC (permalink / raw)
To: kyle, lifeng1519, dimastep, marcandre.lureau,
Daniel P. Berrangé,
open list:All patches CC here
Cc: Li Feng
Root cause:
From `man recvmsg`, the RETURN VALUE says:
These calls return the number of bytes received, or -1 if an error occurred.
In the event of an error, errno is set to indicate the error.
The return value will be 0 when the peer has performed an orderly shutdown.
When an error happens, the socket will be closed, and recvmsg return 0,
then error_setg will trigger a crash.
This unit test could reproduce this issue:
tests/test-char -p /char/socket/client/reconnect-error/unix
The core file backtrace is :
(gdb) bt
#0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
#1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
#2 0x00005555555aaa94 in error_handle_fatal (errp=<optimized out>, err=0x7fffec0012d0) at util/error.c:40
#3 0x00005555555aab6d in error_setv (errp=0x555555802a08 <error_abort>, src=0x5555555c4280 "io/channel.c", line=148,
func=0x5555555c4580 <__func__.17489> "qio_channel_readv_all", err_class=ERROR_CLASS_GENERIC_ERROR,
fmt=<optimized out>, ap=0x7ffff423bae0, suffix=0x0) at util/error.c:73
#4 0x00005555555aacf0 in error_setg_internal (errp=errp@entry=0x555555802a08 <error_abort>,
src=src@entry=0x5555555c4280 "io/channel.c", line=line@entry=148,
func=func@entry=0x5555555c4580 <__func__.17489> "qio_channel_readv_all",
fmt=fmt@entry=0x5555555c43a0 "Unexpected end-of-file before all bytes were read") at util/error.c:97
#5 0x000055555556c25c in qio_channel_readv_all (ioc=<optimized out>, iov=<optimized out>, niov=<optimized out>,
errp=0x555555802a08 <error_abort>) at io/channel.c:147
#6 0x000055555556c29a in qio_channel_read_all (ioc=<optimized out>, buf=<optimized out>, buflen=<optimized out>,
errp=<optimized out>) at io/channel.c:247
#7 0x000055555556ad22 in char_socket_ping_pong (ioc=0x7fffec0008c0) at tests/test-char.c:732
#8 0x000055555556ae12 in char_socket_client_server_thread (data=data@entry=0x55555582e350) at tests/test-char.c:891
#9 0x00005555555a95b6 in qemu_thread_start (args=<optimized out>) at util/qemu-thread-posix.c:519
#10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
#11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
Signed-off-by: Li Feng <fengli@smartx.com>
---
io/channel.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/io/channel.c b/io/channel.c
index e4376eb0bc..1a4a505f01 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -144,8 +144,6 @@ int qio_channel_readv_all(QIOChannel *ioc,
if (ret == 0) {
ret = -1;
- error_setg(errp,
- "Unexpected end-of-file before all bytes were read");
} else if (ret == 1) {
ret = 0;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start
[not found] ` <20200508051441.8143-1-fengli@smartx.com>
2020-05-08 5:14 ` [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0 Li Feng
@ 2020-05-08 5:14 ` Li Feng
2020-05-08 11:00 ` Marc-André Lureau
1 sibling, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-05-08 5:14 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>
---
chardev/char-socket.c | 2 +-
tests/test-char.c | 68 ++++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d5c9049eec 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
+static int
char_socket_ping_pong(QIOChannel *ioc)
{
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), &error_abort);
+ 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);
-
object_unref(OBJECT(ioc));
+ return 0;
}
@@ -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)) != 0) {
+ goto retry;
+ }
return NULL;
}
@@ -869,6 +901,7 @@ typedef struct {
const char *reconnect;
bool wait_connected;
bool fd_pass;
+ char_socket_cb event_cb;
} CharSocketClientTestConfig;
static void char_socket_client_dupid_test(gconstpointer opaque)
@@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
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;
@@ -983,8 +1017,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);
@@ -1022,7 +1057,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;
@@ -1467,19 +1502,22 @@ 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", false, false }; \
+ { addr, ",reconnect=1", false, false, char_socket_event }; \
+ static CharSocketClientTestConfig client8 ## 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, \
@@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
&client6 ##name, char_socket_client_test); \
g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
- &client7 ##name, char_socket_client_dupid_test)
+ &client7 ##name, char_socket_client_dupid_test);\
+ g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
+ &client8 ##name, char_socket_client_test)
if (has_ipv4) {
SOCKET_SERVER_TEST(tcp, &tcpaddr);
--
2.11.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-05-08 5:14 ` [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
@ 2020-05-08 11:00 ` Marc-André Lureau
2020-05-08 12:16 ` Li Feng
0 siblings, 1 reply; 32+ messages in thread
From: Marc-André Lureau @ 2020-05-08 11:00 UTC (permalink / raw)
To: Li Feng
Cc: Feng Li, Dima Stepanov, Kyle Zhang,
open list:All patches CC here, Paolo Bonzini
On Fri, May 8, 2020 at 7:14 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>
After applying your patch, with qemu configure --enable-debug
--enable-sanitizers:
$ tests/test-char
Unexpected error in qio_channel_readv_all() at
/home/elmarco/src/qq/io/channel.c:147:
Unexpected end-of-file before all bytes were read
[1] 2445287 abort (core dumped) tests/test-char
> ---
> chardev/char-socket.c | 2 +-
> tests/test-char.c | 68 ++++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d5c9049eec 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
> +static int
> char_socket_ping_pong(QIOChannel *ioc)
> {
> 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), &error_abort);
> + 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);
> -
> object_unref(OBJECT(ioc));
> + return 0;
> }
>
>
> @@ -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)) != 0) {
> + goto retry;
> + }
>
> return NULL;
> }
> @@ -869,6 +901,7 @@ typedef struct {
> const char *reconnect;
> bool wait_connected;
> bool fd_pass;
> + char_socket_cb event_cb;
> } CharSocketClientTestConfig;
>
> static void char_socket_client_dupid_test(gconstpointer opaque)
> @@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> 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;
> @@ -983,8 +1017,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);
> @@ -1022,7 +1057,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;
> @@ -1467,19 +1502,22 @@ 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", false, false }; \
> + { addr, ",reconnect=1", false, false, char_socket_event }; \
> + static CharSocketClientTestConfig client8 ## 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, \
> @@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
> g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> &client6 ##name, char_socket_client_test); \
> g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> - &client7 ##name, char_socket_client_dupid_test)
> + &client7 ##name, char_socket_client_dupid_test);\
> + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> + &client8 ##name, char_socket_client_test)
>
> if (has_ipv4) {
> SOCKET_SERVER_TEST(tcp, &tcpaddr);
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-05-08 11:00 ` Marc-André Lureau
@ 2020-05-08 12:16 ` Li Feng
2020-05-08 12:28 ` Marc-André Lureau
0 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-05-08 12:16 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Feng Li, Dima Stepanov, Kyle Zhang,
open list:All patches CC here, Paolo Bonzini
Have you applied the first one?
io/channel: fix crash when qio_channel_readv_all return 0
Thanks,
Feng Li
Marc-André Lureau <marcandre.lureau@redhat.com> 于2020年5月8日周五 下午7:01写道：
>
> On Fri, May 8, 2020 at 7:14 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>
>
> After applying your patch, with qemu configure --enable-debug
> --enable-sanitizers:
>
> $ tests/test-char
> Unexpected error in qio_channel_readv_all() at
> /home/elmarco/src/qq/io/channel.c:147:
> Unexpected end-of-file before all bytes were read
> [1] 2445287 abort (core dumped) tests/test-char
>
> > ---
> > chardev/char-socket.c | 2 +-
> > tests/test-char.c | 68 ++++++++++++++++++++++++++++++++++++++++-----------
> > 2 files changed, 55 insertions(+), 15 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d5c9049eec 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
> > +static int
> > char_socket_ping_pong(QIOChannel *ioc)
> > {
> > 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), &error_abort);
> > + 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);
> > -
> > object_unref(OBJECT(ioc));
> > + return 0;
> > }
> >
> >
> > @@ -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)) != 0) {
> > + goto retry;
> > + }
> >
> > return NULL;
> > }
> > @@ -869,6 +901,7 @@ typedef struct {
> > const char *reconnect;
> > bool wait_connected;
> > bool fd_pass;
> > + char_socket_cb event_cb;
> > } CharSocketClientTestConfig;
> >
> > static void char_socket_client_dupid_test(gconstpointer opaque)
> > @@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> > 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;
> > @@ -983,8 +1017,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);
> > @@ -1022,7 +1057,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;
> > @@ -1467,19 +1502,22 @@ 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", false, false }; \
> > + { addr, ",reconnect=1", false, false, char_socket_event }; \
> > + static CharSocketClientTestConfig client8 ## 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, \
> > @@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
> > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> > &client6 ##name, char_socket_client_test); \
> > g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> > - &client7 ##name, char_socket_client_dupid_test)
> > + &client7 ##name, char_socket_client_dupid_test);\
> > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> > + &client8 ##name, char_socket_client_test)
> >
> > if (has_ipv4) {
> > SOCKET_SERVER_TEST(tcp, &tcpaddr);
> > --
> > 2.11.0
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-05-08 12:16 ` Li Feng
@ 2020-05-08 12:28 ` Marc-André Lureau
2020-05-08 12:31 ` Li Feng
0 siblings, 1 reply; 32+ messages in thread
From: Marc-André Lureau @ 2020-05-08 12:28 UTC (permalink / raw)
To: Li Feng
Cc: Feng Li, Kyle Zhang, Dima Stepanov,
open list:All patches CC here, Paolo Bonzini
Hi
On Fri, May 8, 2020 at 2:16 PM Li Feng <fengli@smartx.com> wrote:
>
> Have you applied the first one?
> io/channel: fix crash when qio_channel_readv_all return 0
>
Sorry, I missed that one. With that it passes. thanks
> Thanks,
>
> Feng Li
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 于2020年5月8日周五 下午7:01写道：
>
> >
> > On Fri, May 8, 2020 at 7:14 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>
> >
> > After applying your patch, with qemu configure --enable-debug
> > --enable-sanitizers:
> >
> > $ tests/test-char
> > Unexpected error in qio_channel_readv_all() at
> > /home/elmarco/src/qq/io/channel.c:147:
> > Unexpected end-of-file before all bytes were read
> > [1] 2445287 abort (core dumped) tests/test-char
> >
> > > ---
> > > chardev/char-socket.c | 2 +-
> > > tests/test-char.c | 68 ++++++++++++++++++++++++++++++++++++++++-----------
> > > 2 files changed, 55 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d5c9049eec 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
> > > +static int
> > > char_socket_ping_pong(QIOChannel *ioc)
> > > {
> > > 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), &error_abort);
> > > + 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);
> > > -
> > > object_unref(OBJECT(ioc));
> > > + return 0;
> > > }
> > >
> > >
> > > @@ -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)) != 0) {
> > > + goto retry;
> > > + }
> > >
> > > return NULL;
> > > }
> > > @@ -869,6 +901,7 @@ typedef struct {
> > > const char *reconnect;
> > > bool wait_connected;
> > > bool fd_pass;
> > > + char_socket_cb event_cb;
> > > } CharSocketClientTestConfig;
> > >
> > > static void char_socket_client_dupid_test(gconstpointer opaque)
> > > @@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> > > 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;
> > > @@ -983,8 +1017,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);
> > > @@ -1022,7 +1057,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;
> > > @@ -1467,19 +1502,22 @@ 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", false, false }; \
> > > + { addr, ",reconnect=1", false, false, char_socket_event }; \
> > > + static CharSocketClientTestConfig client8 ## 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, \
> > > @@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
> > > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> > > &client6 ##name, char_socket_client_test); \
> > > g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> > > - &client7 ##name, char_socket_client_dupid_test)
> > > + &client7 ##name, char_socket_client_dupid_test);\
> > > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> > > + &client8 ##name, char_socket_client_test)
> > >
> > > if (has_ipv4) {
> > > SOCKET_SERVER_TEST(tcp, &tcpaddr);
> > > --
> > > 2.11.0
> > >
> >
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start
2020-05-08 12:28 ` Marc-André Lureau
@ 2020-05-08 12:31 ` Li Feng
0 siblings, 0 replies; 32+ messages in thread
From: Li Feng @ 2020-05-08 12:31 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Feng Li, Kyle Zhang, Dima Stepanov,
open list:All patches CC here, Paolo Bonzini
Thanks!
Feng Li
Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年5月8日周五 下午8:29写道：
>
> Hi
>
> On Fri, May 8, 2020 at 2:16 PM Li Feng <fengli@smartx.com> wrote:
> >
> > Have you applied the first one?
> > io/channel: fix crash when qio_channel_readv_all return 0
> >
>
> Sorry, I missed that one. With that it passes. thanks
>
> > Thanks,
> >
> > Feng Li
> >
> > Marc-André Lureau <marcandre.lureau@redhat.com> 于2020年5月8日周五 下午7:01写道：
> >
> > >
> > > On Fri, May 8, 2020 at 7:14 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>
> > >
> > > After applying your patch, with qemu configure --enable-debug
> > > --enable-sanitizers:
> > >
> > > $ tests/test-char
> > > Unexpected error in qio_channel_readv_all() at
> > > /home/elmarco/src/qq/io/channel.c:147:
> > > Unexpected end-of-file before all bytes were read
> > > [1] 2445287 abort (core dumped) tests/test-char
> > >
> > > > ---
> > > > chardev/char-socket.c | 2 +-
> > > > tests/test-char.c | 68 ++++++++++++++++++++++++++++++++++++++++-----------
> > > > 2 files changed, 55 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d5c9049eec 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
> > > > +static int
> > > > char_socket_ping_pong(QIOChannel *ioc)
> > > > {
> > > > 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), &error_abort);
> > > > + 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);
> > > > -
> > > > object_unref(OBJECT(ioc));
> > > > + return 0;
> > > > }
> > > >
> > > >
> > > > @@ -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)) != 0) {
> > > > + goto retry;
> > > > + }
> > > >
> > > > return NULL;
> > > > }
> > > > @@ -869,6 +901,7 @@ typedef struct {
> > > > const char *reconnect;
> > > > bool wait_connected;
> > > > bool fd_pass;
> > > > + char_socket_cb event_cb;
> > > > } CharSocketClientTestConfig;
> > > >
> > > > static void char_socket_client_dupid_test(gconstpointer opaque)
> > > > @@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> > > > 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;
> > > > @@ -983,8 +1017,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);
> > > > @@ -1022,7 +1057,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;
> > > > @@ -1467,19 +1502,22 @@ 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", false, false }; \
> > > > + { addr, ",reconnect=1", false, false, char_socket_event }; \
> > > > + static CharSocketClientTestConfig client8 ## 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, \
> > > > @@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
> > > > g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> > > > &client6 ##name, char_socket_client_test); \
> > > > g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> > > > - &client7 ##name, char_socket_client_dupid_test)
> > > > + &client7 ##name, char_socket_client_dupid_test);\
> > > > + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> > > > + &client8 ##name, char_socket_client_test)
> > > >
> > > > if (has_ipv4) {
> > > > SOCKET_SERVER_TEST(tcp, &tcpaddr);
> > > > --
> > > > 2.11.0
> > > >
> > >
> >
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0
2020-05-08 5:14 ` [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0 Li Feng
@ 2020-05-08 12:32 ` Marc-André Lureau
2020-05-08 12:42 ` Li Feng
0 siblings, 1 reply; 32+ messages in thread
From: Marc-André Lureau @ 2020-05-08 12:32 UTC (permalink / raw)
To: Li Feng
Cc: Li Feng, Dima Stepanov, Kyle Zhang, Daniel P. Berrangé,
open list:All patches CC here
Hi
On Fri, May 8, 2020 at 7:14 AM Li Feng <fengli@smartx.com> wrote:
>
> Root cause:
> From `man recvmsg`, the RETURN VALUE says:
> These calls return the number of bytes received, or -1 if an error occurred.
> In the event of an error, errno is set to indicate the error.
> The return value will be 0 when the peer has performed an orderly shutdown.
>
> When an error happens, the socket will be closed, and recvmsg return 0,
> then error_setg will trigger a crash.
>
> This unit test could reproduce this issue:
> tests/test-char -p /char/socket/client/reconnect-error/unix
Current master doesn't trigger the backtrace, it's only after your patch 2.
>
> The core file backtrace is :
>
> (gdb) bt
> #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
> #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
> #2 0x00005555555aaa94 in error_handle_fatal (errp=<optimized out>, err=0x7fffec0012d0) at util/error.c:40
> #3 0x00005555555aab6d in error_setv (errp=0x555555802a08 <error_abort>, src=0x5555555c4280 "io/channel.c", line=148,
> func=0x5555555c4580 <__func__.17489> "qio_channel_readv_all", err_class=ERROR_CLASS_GENERIC_ERROR,
> fmt=<optimized out>, ap=0x7ffff423bae0, suffix=0x0) at util/error.c:73
> #4 0x00005555555aacf0 in error_setg_internal (errp=errp@entry=0x555555802a08 <error_abort>,
> src=src@entry=0x5555555c4280 "io/channel.c", line=line@entry=148,
> func=func@entry=0x5555555c4580 <__func__.17489> "qio_channel_readv_all",
> fmt=fmt@entry=0x5555555c43a0 "Unexpected end-of-file before all bytes were read") at util/error.c:97
> #5 0x000055555556c25c in qio_channel_readv_all (ioc=<optimized out>, iov=<optimized out>, niov=<optimized out>,
> errp=0x555555802a08 <error_abort>) at io/channel.c:147
> #6 0x000055555556c29a in qio_channel_read_all (ioc=<optimized out>, buf=<optimized out>, buflen=<optimized out>,
> errp=<optimized out>) at io/channel.c:247
> #7 0x000055555556ad22 in char_socket_ping_pong (ioc=0x7fffec0008c0) at tests/test-char.c:732
> #8 0x000055555556ae12 in char_socket_client_server_thread (data=data@entry=0x55555582e350) at tests/test-char.c:891
> #9 0x00005555555a95b6 in qemu_thread_start (args=<optimized out>) at util/qemu-thread-posix.c:519
> #10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
> #11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> io/channel.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/io/channel.c b/io/channel.c
> index e4376eb0bc..1a4a505f01 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -144,8 +144,6 @@ int qio_channel_readv_all(QIOChannel *ioc,
>
> if (ret == 0) {
> ret = -1;
> - error_setg(errp,
> - "Unexpected end-of-file before all bytes were read");
Nack, this code is fine.
The problem is that the test case doesn't expect a disconnect in
char_socket_ping_pong().
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0
2020-05-08 12:32 ` Marc-André Lureau
@ 2020-05-08 12:42 ` Li Feng
2020-05-11 10:02 ` Daniel P. Berrangé
0 siblings, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-05-08 12:42 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Li Feng, Dima Stepanov, Kyle Zhang, Daniel P. Berrangé,
open list:All patches CC here
Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年5月8日周五 下午8:32写道：
>
> Hi
>
> On Fri, May 8, 2020 at 7:14 AM Li Feng <fengli@smartx.com> wrote:
> >
> > Root cause:
> > From `man recvmsg`, the RETURN VALUE says:
> > These calls return the number of bytes received, or -1 if an error occurred.
> > In the event of an error, errno is set to indicate the error.
> > The return value will be 0 when the peer has performed an orderly shutdown.
> >
> > When an error happens, the socket will be closed, and recvmsg return 0,
> > then error_setg will trigger a crash.
> >
> > This unit test could reproduce this issue:
> > tests/test-char -p /char/socket/client/reconnect-error/unix
>
> Current master doesn't trigger the backtrace, it's only after your patch 2.
Yes. However, the issue did exist in the master code base.
The test case in patch 2 revealed this issue.
>
> >
> > The core file backtrace is :
> >
> > (gdb) bt
> > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
> > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
> > #2 0x00005555555aaa94 in error_handle_fatal (errp=<optimized out>, err=0x7fffec0012d0) at util/error.c:40
> > #3 0x00005555555aab6d in error_setv (errp=0x555555802a08 <error_abort>, src=0x5555555c4280 "io/channel.c", line=148,
> > func=0x5555555c4580 <__func__.17489> "qio_channel_readv_all", err_class=ERROR_CLASS_GENERIC_ERROR,
> > fmt=<optimized out>, ap=0x7ffff423bae0, suffix=0x0) at util/error.c:73
> > #4 0x00005555555aacf0 in error_setg_internal (errp=errp@entry=0x555555802a08 <error_abort>,
> > src=src@entry=0x5555555c4280 "io/channel.c", line=line@entry=148,
> > func=func@entry=0x5555555c4580 <__func__.17489> "qio_channel_readv_all",
> > fmt=fmt@entry=0x5555555c43a0 "Unexpected end-of-file before all bytes were read") at util/error.c:97
> > #5 0x000055555556c25c in qio_channel_readv_all (ioc=<optimized out>, iov=<optimized out>, niov=<optimized out>,
> > errp=0x555555802a08 <error_abort>) at io/channel.c:147
> > #6 0x000055555556c29a in qio_channel_read_all (ioc=<optimized out>, buf=<optimized out>, buflen=<optimized out>,
> > errp=<optimized out>) at io/channel.c:247
> > #7 0x000055555556ad22 in char_socket_ping_pong (ioc=0x7fffec0008c0) at tests/test-char.c:732
> > #8 0x000055555556ae12 in char_socket_client_server_thread (data=data@entry=0x55555582e350) at tests/test-char.c:891
> > #9 0x00005555555a95b6 in qemu_thread_start (args=<optimized out>) at util/qemu-thread-posix.c:519
> > #10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
> > #11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > io/channel.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/io/channel.c b/io/channel.c
> > index e4376eb0bc..1a4a505f01 100644
> > --- a/io/channel.c
> > +++ b/io/channel.c
> > @@ -144,8 +144,6 @@ int qio_channel_readv_all(QIOChannel *ioc,
> >
> > if (ret == 0) {
> > ret = -1;
> > - error_setg(errp,
> > - "Unexpected end-of-file before all bytes were read");
>
> Nack, this code is fine.
>
> The problem is that the test case doesn't expect a disconnect in
> char_socket_ping_pong().
Yes, in the test case I try to inject an error in char_socket_ping_pong.
Any concerns about these two patches?
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0
2020-05-08 12:42 ` Li Feng
@ 2020-05-11 10:02 ` Daniel P. Berrangé
2020-05-11 12:09 ` Li Feng
0 siblings, 1 reply; 32+ messages in thread
From: Daniel P. Berrangé @ 2020-05-11 10:02 UTC (permalink / raw)
To: Li Feng
Cc: Li Feng, Kyle Zhang, Marc-André Lureau,
open list:All patches CC here, Dima Stepanov
On Fri, May 08, 2020 at 08:42:22PM +0800, Li Feng wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年5月8日周五 下午8:32写道：
> >
> > Hi
> >
> > On Fri, May 8, 2020 at 7:14 AM Li Feng <fengli@smartx.com> wrote:
> > >
> > > Root cause:
> > > From `man recvmsg`, the RETURN VALUE says:
> > > These calls return the number of bytes received, or -1 if an error occurred.
> > > In the event of an error, errno is set to indicate the error.
> > > The return value will be 0 when the peer has performed an orderly shutdown.
> > >
> > > When an error happens, the socket will be closed, and recvmsg return 0,
> > > then error_setg will trigger a crash.
> > >
> > > This unit test could reproduce this issue:
> > > tests/test-char -p /char/socket/client/reconnect-error/unix
> >
> > Current master doesn't trigger the backtrace, it's only after your patch 2.
> Yes. However, the issue did exist in the master code base.
> The test case in patch 2 revealed this issue.
>
> >
> > >
> > > The core file backtrace is :
> > >
> > > (gdb) bt
> > > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
> > > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
> > > #2 0x00005555555aaa94 in error_handle_fatal (errp=<optimized out>, err=0x7fffec0012d0) at util/error.c:40
> > > #3 0x00005555555aab6d in error_setv (errp=0x555555802a08 <error_abort>, src=0x5555555c4280 "io/channel.c", line=148,
> > > func=0x5555555c4580 <__func__.17489> "qio_channel_readv_all", err_class=ERROR_CLASS_GENERIC_ERROR,
> > > fmt=<optimized out>, ap=0x7ffff423bae0, suffix=0x0) at util/error.c:73
> > > #4 0x00005555555aacf0 in error_setg_internal (errp=errp@entry=0x555555802a08 <error_abort>,
> > > src=src@entry=0x5555555c4280 "io/channel.c", line=line@entry=148,
> > > func=func@entry=0x5555555c4580 <__func__.17489> "qio_channel_readv_all",
> > > fmt=fmt@entry=0x5555555c43a0 "Unexpected end-of-file before all bytes were read") at util/error.c:97
> > > #5 0x000055555556c25c in qio_channel_readv_all (ioc=<optimized out>, iov=<optimized out>, niov=<optimized out>,
> > > errp=0x555555802a08 <error_abort>) at io/channel.c:147
> > > #6 0x000055555556c29a in qio_channel_read_all (ioc=<optimized out>, buf=<optimized out>, buflen=<optimized out>,
> > > errp=<optimized out>) at io/channel.c:247
> > > #7 0x000055555556ad22 in char_socket_ping_pong (ioc=0x7fffec0008c0) at tests/test-char.c:732
> > > #8 0x000055555556ae12 in char_socket_client_server_thread (data=data@entry=0x55555582e350) at tests/test-char.c:891
> > > #9 0x00005555555a95b6 in qemu_thread_start (args=<optimized out>) at util/qemu-thread-posix.c:519
> > > #10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
> > > #11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
> > >
> > > Signed-off-by: Li Feng <fengli@smartx.com>
> > > ---
> > > io/channel.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/io/channel.c b/io/channel.c
> > > index e4376eb0bc..1a4a505f01 100644
> > > --- a/io/channel.c
> > > +++ b/io/channel.c
> > > @@ -144,8 +144,6 @@ int qio_channel_readv_all(QIOChannel *ioc,
> > >
> > > if (ret == 0) {
> > > ret = -1;
> > > - error_setg(errp,
> > > - "Unexpected end-of-file before all bytes were read");
> >
> > Nack, this code is fine.
> >
> > The problem is that the test case doesn't expect a disconnect in
> > char_socket_ping_pong().
> Yes, in the test case I try to inject an error in char_socket_ping_pong.
> Any concerns about these two patches?
I agree with Marc-André - this patch is wrong. qio_channel_readv_all
*MUST* always set 'errp' if the return value is -1. To not set 'errp'
is violating the calling convention.
The bug here is in your test case - it is passing '&error_abort' to the
'qio_channel_readv_all' call. If you don't want the test to crash, then
don't pass &error_abort
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0
2020-05-11 10:02 ` Daniel P. Berrangé
@ 2020-05-11 12:09 ` Li Feng
0 siblings, 0 replies; 32+ messages in thread
From: Li Feng @ 2020-05-11 12:09 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Li Feng, Kyle Zhang, Marc-André Lureau,
open list:All patches CC here, Dima Stepanov
Thank you, Daniel and Marc-André.
I understand now. The error_abort is used to abort the program.
I should rewrite the char_socket_ping_pong.
I will post a new version without this wrong patch.
Thanks,
Feng Li
Daniel P. Berrangé <berrange@redhat.com> 于2020年5月11日周一 下午6:03写道：
>
> On Fri, May 08, 2020 at 08:42:22PM +0800, Li Feng wrote:
> > Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年5月8日周五 下午8:32写道：
> > >
> > > Hi
> > >
> > > On Fri, May 8, 2020 at 7:14 AM Li Feng <fengli@smartx.com> wrote:
> > > >
> > > > Root cause:
> > > > From `man recvmsg`, the RETURN VALUE says:
> > > > These calls return the number of bytes received, or -1 if an error occurred.
> > > > In the event of an error, errno is set to indicate the error.
> > > > The return value will be 0 when the peer has performed an orderly shutdown.
> > > >
> > > > When an error happens, the socket will be closed, and recvmsg return 0,
> > > > then error_setg will trigger a crash.
> > > >
> > > > This unit test could reproduce this issue:
> > > > tests/test-char -p /char/socket/client/reconnect-error/unix
> > >
> > > Current master doesn't trigger the backtrace, it's only after your patch 2.
> > Yes. However, the issue did exist in the master code base.
> > The test case in patch 2 revealed this issue.
> >
> > >
> > > >
> > > > The core file backtrace is :
> > > >
> > > > (gdb) bt
> > > > #0 0x00007ffff5ac3277 in raise () from /lib64/libc.so.6
> > > > #1 0x00007ffff5ac4968 in abort () from /lib64/libc.so.6
> > > > #2 0x00005555555aaa94 in error_handle_fatal (errp=<optimized out>, err=0x7fffec0012d0) at util/error.c:40
> > > > #3 0x00005555555aab6d in error_setv (errp=0x555555802a08 <error_abort>, src=0x5555555c4280 "io/channel.c", line=148,
> > > > func=0x5555555c4580 <__func__.17489> "qio_channel_readv_all", err_class=ERROR_CLASS_GENERIC_ERROR,
> > > > fmt=<optimized out>, ap=0x7ffff423bae0, suffix=0x0) at util/error.c:73
> > > > #4 0x00005555555aacf0 in error_setg_internal (errp=errp@entry=0x555555802a08 <error_abort>,
> > > > src=src@entry=0x5555555c4280 "io/channel.c", line=line@entry=148,
> > > > func=func@entry=0x5555555c4580 <__func__.17489> "qio_channel_readv_all",
> > > > fmt=fmt@entry=0x5555555c43a0 "Unexpected end-of-file before all bytes were read") at util/error.c:97
> > > > #5 0x000055555556c25c in qio_channel_readv_all (ioc=<optimized out>, iov=<optimized out>, niov=<optimized out>,
> > > > errp=0x555555802a08 <error_abort>) at io/channel.c:147
> > > > #6 0x000055555556c29a in qio_channel_read_all (ioc=<optimized out>, buf=<optimized out>, buflen=<optimized out>,
> > > > errp=<optimized out>) at io/channel.c:247
> > > > #7 0x000055555556ad22 in char_socket_ping_pong (ioc=0x7fffec0008c0) at tests/test-char.c:732
> > > > #8 0x000055555556ae12 in char_socket_client_server_thread (data=data@entry=0x55555582e350) at tests/test-char.c:891
> > > > #9 0x00005555555a95b6 in qemu_thread_start (args=<optimized out>) at util/qemu-thread-posix.c:519
> > > > #10 0x00007ffff5e61e25 in start_thread () from /lib64/libpthread.so.0
> > > > #11 0x00007ffff5b8bbad in clone () from /lib64/libc.so.6
> > > >
> > > > Signed-off-by: Li Feng <fengli@smartx.com>
> > > > ---
> > > > io/channel.c | 2 --
> > > > 1 file changed, 2 deletions(-)
> > > >
> > > > diff --git a/io/channel.c b/io/channel.c
> > > > index e4376eb0bc..1a4a505f01 100644
> > > > --- a/io/channel.c
> > > > +++ b/io/channel.c
> > > > @@ -144,8 +144,6 @@ int qio_channel_readv_all(QIOChannel *ioc,
> > > >
> > > > if (ret == 0) {
> > > > ret = -1;
> > > > - error_setg(errp,
> > > > - "Unexpected end-of-file before all bytes were read");
> > >
> > > Nack, this code is fine.
> > >
> > > The problem is that the test case doesn't expect a disconnect in
> > > char_socket_ping_pong().
> > Yes, in the test case I try to inject an error in char_socket_ping_pong.
> > Any concerns about these two patches?
>
> I agree with Marc-André - this patch is wrong. qio_channel_readv_all
> *MUST* always set 'errp' if the return value is -1. To not set 'errp'
> is violating the calling convention.
>
> The bug here is in your test case - it is passing '&error_abort' to the
> 'qio_channel_readv_all' call. If you don't want the test to crash, then
> don't pass &error_abort
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v4] char-socket: initialize reconnect timer only when the timer doesn't start
2020-04-28 8:51 ` [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
[not found] ` <20200508051441.8143-1-fengli@smartx.com>
@ 2020-05-11 12:39 ` Li Feng
2020-05-21 15:15 ` Paolo Bonzini
1 sibling, 1 reply; 32+ messages in thread
From: Li Feng @ 2020-05-11 12:39 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>
---
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 | 74 +++++++++++++++++++++++++++++++++++++++------------
2 files changed, 58 insertions(+), 18 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d94580c61e 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,6 +901,7 @@ typedef struct {
const char *reconnect;
bool wait_connected;
bool fd_pass;
+ char_socket_cb event_cb;
} CharSocketClientTestConfig;
static void char_socket_client_dupid_test(gconstpointer opaque)
@@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
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;
@@ -983,8 +1017,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);
@@ -1022,7 +1057,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;
@@ -1467,19 +1502,22 @@ 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", false, false }; \
+ { addr, ",reconnect=1", false, false, char_socket_event }; \
+ static CharSocketClientTestConfig client8 ## 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, \
@@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
&client6 ##name, char_socket_client_test); \
g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
- &client7 ##name, char_socket_client_dupid_test)
+ &client7 ##name, char_socket_client_dupid_test);\
+ g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
+ &client8 ##name, char_socket_client_test)
if (has_ipv4) {
SOCKET_SERVER_TEST(tcp, &tcpaddr);
--
2.11.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v4] char-socket: initialize reconnect timer only when the timer doesn't start
2020-05-11 12:39 ` [PATCH v4] " Li Feng
@ 2020-05-21 15:15 ` Paolo Bonzini
0 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2020-05-21 15:15 UTC (permalink / raw)
To: Li Feng, kyle, lifeng1519, dimastep, marcandre.lureau,
open list:All patches CC here
Sorry, can you resend this?
I cannot apply it.
Paolo
On 11/05/20 14:39, 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>
> ---
> 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 | 74 +++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 58 insertions(+), 18 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1f14c2c7c8..d84330b3c9 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 8d39bdc9fa..d94580c61e 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,6 +901,7 @@ typedef struct {
> const char *reconnect;
> bool wait_connected;
> bool fd_pass;
> + char_socket_cb event_cb;
> } CharSocketClientTestConfig;
>
> static void char_socket_client_dupid_test(gconstpointer opaque)
> @@ -920,6 +953,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque)
> 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;
> @@ -983,8 +1017,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);
> @@ -1022,7 +1057,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;
> @@ -1467,19 +1502,22 @@ 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", false, false }; \
> + { addr, ",reconnect=1", false, false, char_socket_event }; \
> + static CharSocketClientTestConfig client8 ## 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, \
> @@ -1493,7 +1531,9 @@ int main(int argc, char **argv)
> g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \
> &client6 ##name, char_socket_client_test); \
> g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \
> - &client7 ##name, char_socket_client_dupid_test)
> + &client7 ##name, char_socket_client_dupid_test);\
> + g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \
> + &client8 ##name, char_socket_client_test)
>
> if (has_ipv4) {
> SOCKET_SERVER_TEST(tcp, &tcpaddr);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2020-05-21 15:16 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15 3:28 [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Li Feng
2020-04-15 3:28 ` [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect Li Feng
2020-04-14 1:24 ` Raphael Norwitz
2020-04-20 11:18 ` Li Feng
2020-04-15 3:28 ` [PATCH 2/4] vhost-user-blk: fix invalid memory access Li Feng
2020-04-14 1:44 ` Raphael Norwitz
2020-04-15 3:28 ` [PATCH 3/4] char-socket: avoid double call tcp_chr_free_connection Li Feng
2020-04-15 10:35 ` Marc-André Lureau
2020-04-20 3:10 ` Li Feng
2020-04-20 14:28 ` Marc-André Lureau
2020-04-28 8:51 ` [PATCH v2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
[not found] ` <20200508051441.8143-1-fengli@smartx.com>
2020-05-08 5:14 ` [PATCH v3 1/2] io/channel: fix crash when qio_channel_readv_all return 0 Li Feng
2020-05-08 12:32 ` Marc-André Lureau
2020-05-08 12:42 ` Li Feng
2020-05-11 10:02 ` Daniel P. Berrangé
2020-05-11 12:09 ` Li Feng
2020-05-08 5:14 ` [PATCH v3 2/2] char-socket: initialize reconnect timer only when the timer doesn't start Li Feng
2020-05-08 11:00 ` Marc-André Lureau
2020-05-08 12:16 ` Li Feng
2020-05-08 12:28 ` Marc-André Lureau
2020-05-08 12:31 ` Li Feng
2020-05-11 12:39 ` [PATCH v4] " Li Feng
2020-05-21 15:15 ` Paolo Bonzini
2020-04-28 8:54 ` [PATCH v2] " Li Feng
2020-04-28 8:59 ` Li Feng
2020-04-28 11:05 ` Marc-André Lureau
2020-04-28 14:09 ` Feng Li
2020-04-15 3:28 ` [PATCH 4/4] vhost-user-blk: fix crash in realize process Li Feng
2020-04-14 1:54 ` Raphael Norwitz
[not found] ` <CAHckoCwsLwqNhVHK4PF+pzt5ExkurPNsWLw7ueRG8dfOUA2rXg@mail.gmail.com>
2020-04-17 10:07 ` Fwd: " Li Feng
2020-04-17 9:45 ` [PATCH 0/4] fix crashes when inject errors to vhost-user-blk chardev Michael S. Tsirkin
2020-04-17 10:11 ` Li Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.