| Message ID | 1457320380-5111-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote: > The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: > > Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) > > are available in the git repository at: > > https://github.com/jasowang/qemu.git tags/net-pull-request > > for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: > > net: check packet payload length (2016-03-07 10:15:48 +0800) > > ---------------------------------------------------------------- > > - a new netfilter implementation: mirror > - netfilter could be disabled and enabled through qom-set now > - fix netfilter crash when specifiying wrong parameters > - rocker switch now can allow user to specifiy world > - fix OOB access for ne2000 Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64): TEST: tests/test-netfilter... (pid=26854) /i386/netfilter/addremove_one: OK /i386/netfilter/remove_netdev_one: OK /i386/netfilter/addremove_multi: OK /i386/netfilter/remove_netdev_multi: OK PASS: tests/test-netfilter TEST: tests/test-filter-mirror... (pid=26858) /i386/netfilter/mirror: (consistently, every time I run make check, on the same test). thanks -- PMM
On 03/08/2016 12:51 PM, Peter Maydell wrote: > On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote: >> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: >> >> Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) >> >> are available in the git repository at: >> >> https://github.com/jasowang/qemu.git tags/net-pull-request >> >> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: >> >> net: check packet payload length (2016-03-07 10:15:48 +0800) >> >> ---------------------------------------------------------------- >> >> - a new netfilter implementation: mirror >> - netfilter could be disabled and enabled through qom-set now >> - fix netfilter crash when specifiying wrong parameters >> - rocker switch now can allow user to specifiy world >> - fix OOB access for ne2000 > Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64): > > TEST: tests/test-netfilter... (pid=26854) > /i386/netfilter/addremove_one: OK > /i386/netfilter/remove_netdev_one: OK > /i386/netfilter/addremove_multi: OK > /i386/netfilter/remove_netdev_multi: OK > PASS: tests/test-netfilter > TEST: tests/test-filter-mirror... (pid=26858) > /i386/netfilter/mirror: > > (consistently, every time I run make check, on the same test). > > thanks > -- PMM Sorry, it manages to pass on my machine before submitting the pull request. But when I re-try this several times, it fails. This probably means we have bug in mirror implementation. Chen and Congyang, please try to fix this bug and resubmit a new version of the patch. Will drop mirror from this pull request and submit a V2. Thanks
On 03/08/2016 03:33 PM, Jason Wang wrote: > > > On 03/08/2016 12:51 PM, Peter Maydell wrote: >> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote: >>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: >>> >>> Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) >>> >>> are available in the git repository at: >>> >>> https://github.com/jasowang/qemu.git tags/net-pull-request >>> >>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: >>> >>> net: check packet payload length (2016-03-07 10:15:48 +0800) >>> >>> ---------------------------------------------------------------- >>> >>> - a new netfilter implementation: mirror >>> - netfilter could be disabled and enabled through qom-set now >>> - fix netfilter crash when specifiying wrong parameters >>> - rocker switch now can allow user to specifiy world >>> - fix OOB access for ne2000 >> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64): >> >> TEST: tests/test-netfilter... (pid=26854) >> /i386/netfilter/addremove_one: OK >> /i386/netfilter/remove_netdev_one: OK >> /i386/netfilter/addremove_multi: OK >> /i386/netfilter/remove_netdev_multi: OK >> PASS: tests/test-netfilter >> TEST: tests/test-filter-mirror... (pid=26858) >> /i386/netfilter/mirror: >> >> (consistently, every time I run make check, on the same test). >> >> thanks >> -- PMM > > Sorry, it manages to pass on my machine before submitting the pull > request. But when I re-try this several times, it fails. > > This probably means we have bug in mirror implementation. Chen and > Congyang, please try to fix this bug and resubmit a new version of the > patch. > > Will drop mirror from this pull request and submit a V2. OK. what is the version of the kernel that you use? Thanks Wen Congyang > > Thanks > > > . >
On 03/08/2016 03:50 PM, Wen Congyang wrote: > On 03/08/2016 03:33 PM, Jason Wang wrote: >> >> On 03/08/2016 12:51 PM, Peter Maydell wrote: >>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote: >>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: >>>> >>>> Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) >>>> >>>> are available in the git repository at: >>>> >>>> https://github.com/jasowang/qemu.git tags/net-pull-request >>>> >>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: >>>> >>>> net: check packet payload length (2016-03-07 10:15:48 +0800) >>>> >>>> ---------------------------------------------------------------- >>>> >>>> - a new netfilter implementation: mirror >>>> - netfilter could be disabled and enabled through qom-set now >>>> - fix netfilter crash when specifiying wrong parameters >>>> - rocker switch now can allow user to specifiy world >>>> - fix OOB access for ne2000 >>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64): >>> >>> TEST: tests/test-netfilter... (pid=26854) >>> /i386/netfilter/addremove_one: OK >>> /i386/netfilter/remove_netdev_one: OK >>> /i386/netfilter/addremove_multi: OK >>> /i386/netfilter/remove_netdev_multi: OK >>> PASS: tests/test-netfilter >>> TEST: tests/test-filter-mirror... (pid=26858) >>> /i386/netfilter/mirror: >>> >>> (consistently, every time I run make check, on the same test). >>> >>> thanks >>> -- PMM >> Sorry, it manages to pass on my machine before submitting the pull >> request. But when I re-try this several times, it fails. >> >> This probably means we have bug in mirror implementation. Chen and >> Congyang, please try to fix this bug and resubmit a new version of the >> patch. >> >> Will drop mirror from this pull request and submit a V2. > OK. what is the version of the kernel that you use? 4.2 but probably unrelated. Gdb shows the test wait at recv(). > > Thanks > Wen Congyang > >> Thanks >> >> >> . >> > >
On 03/08/2016 03:56 PM, Jason Wang wrote: > > On 03/08/2016 03:50 PM, Wen Congyang wrote: >> On 03/08/2016 03:33 PM, Jason Wang wrote: >>> On 03/08/2016 12:51 PM, Peter Maydell wrote: >>>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote: >>>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: >>>>> >>>>> Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) >>>>> >>>>> are available in the git repository at: >>>>> >>>>> https://github.com/jasowang/qemu.git tags/net-pull-request >>>>> >>>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: >>>>> >>>>> net: check packet payload length (2016-03-07 10:15:48 +0800) >>>>> >>>>> ---------------------------------------------------------------- >>>>> >>>>> - a new netfilter implementation: mirror >>>>> - netfilter could be disabled and enabled through qom-set now >>>>> - fix netfilter crash when specifiying wrong parameters >>>>> - rocker switch now can allow user to specifiy world >>>>> - fix OOB access for ne2000 >>>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64): >>>> >>>> TEST: tests/test-netfilter... (pid=26854) >>>> /i386/netfilter/addremove_one: OK >>>> /i386/netfilter/remove_netdev_one: OK >>>> /i386/netfilter/addremove_multi: OK >>>> /i386/netfilter/remove_netdev_multi: OK >>>> PASS: tests/test-netfilter >>>> TEST: tests/test-filter-mirror... (pid=26858) >>>> /i386/netfilter/mirror: >>>> >>>> (consistently, every time I run make check, on the same test). >>>> >>>> thanks >>>> -- PMM >>> Sorry, it manages to pass on my machine before submitting the pull >>> request. But when I re-try this several times, it fails. >>> >>> This probably means we have bug in mirror implementation. Chen and >>> Congyang, please try to fix this bug and resubmit a new version of the >>> patch. >>> >>> Will drop mirror from this pull request and submit a V2. >> OK. what is the version of the kernel that you use? > 4.2 but probably unrelated. Gdb shows the test wait at recv(). Hi~ Jason. I found the reason for this problem is that unix_connect() have not connect to sock_path before iov_send(). It need time to establish connection. so can we fix it with usleep() like this: recv_sock = unix_connect(sock_path, NULL); g_assert_cmpint(recv_sock, !=, -1); + usleep(1000); ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf)); g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); close(send_sock[0]); ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >> Thanks >> Wen Congyang >> >>> Thanks >>> >>> >>> . >>> >> > > > . >
On 03/08/2016 05:06 PM, Zhang Chen wrote: > > > On 03/08/2016 03:56 PM, Jason Wang wrote: >> >> On 03/08/2016 03:50 PM, Wen Congyang wrote: >>> On 03/08/2016 03:33 PM, Jason Wang wrote: >>>> On 03/08/2016 12:51 PM, Peter Maydell wrote: >>>>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote: >>>>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: >>>>>> >>>>>> Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) >>>>>> >>>>>> are available in the git repository at: >>>>>> >>>>>> https://github.com/jasowang/qemu.git tags/net-pull-request >>>>>> >>>>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: >>>>>> >>>>>> net: check packet payload length (2016-03-07 10:15:48 +0800) >>>>>> >>>>>> ---------------------------------------------------------------- >>>>>> >>>>>> - a new netfilter implementation: mirror >>>>>> - netfilter could be disabled and enabled through qom-set now >>>>>> - fix netfilter crash when specifiying wrong parameters >>>>>> - rocker switch now can allow user to specifiy world >>>>>> - fix OOB access for ne2000 >>>>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64): >>>>> >>>>> TEST: tests/test-netfilter... (pid=26854) >>>>> /i386/netfilter/addremove_one: OK >>>>> /i386/netfilter/remove_netdev_one: OK >>>>> /i386/netfilter/addremove_multi: OK >>>>> /i386/netfilter/remove_netdev_multi: OK >>>>> PASS: tests/test-netfilter >>>>> TEST: tests/test-filter-mirror... (pid=26858) >>>>> /i386/netfilter/mirror: >>>>> >>>>> (consistently, every time I run make check, on the same test). >>>>> >>>>> thanks >>>>> -- PMM >>>> Sorry, it manages to pass on my machine before submitting the pull >>>> request. But when I re-try this several times, it fails. >>>> >>>> This probably means we have bug in mirror implementation. Chen and >>>> Congyang, please try to fix this bug and resubmit a new version of the >>>> patch. >>>> >>>> Will drop mirror from this pull request and submit a V2. >>> OK. what is the version of the kernel that you use? >> 4.2 but probably unrelated. Gdb shows the test wait at recv(). > > Hi~ Jason. > > I found the reason for this problem is that > unix_connect() have not connect to sock_path before iov_send(). After unix_connect() returns, the connection is established. qemu char device will call qemu_chr_accept() after the connection is established. If we send data before qemu_chr_accept() is called, the data will be dropped by qemu char device: static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len) { TCPCharDriver *s = chr->opaque; if (s->connected) { ... return ret; } else { /* XXX: indicate an error ? */ return len; } } We should wait some to let qemu_chr_accept() is called before sending data. Thanks Wen Congyang > It need time to establish connection. so can we fix it with usleep() > like this: > > recv_sock = unix_connect(sock_path, NULL); > g_assert_cmpint(recv_sock, !=, -1); > + usleep(1000); > > ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf)); > g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); > close(send_sock[0]); > > ret = qemu_recv(recv_sock, &len, sizeof(len), 0); > > > >>> Thanks >>> Wen Congyang >>> >>>> Thanks >>>> >>>> >>>> . >>>> >>> >> >> >> . >> >
On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote: > I found the reason for this problem is that > unix_connect() have not connect to sock_path before iov_send(). > It need time to establish connection. so can we fix it with usleep() > like this: > > recv_sock = unix_connect(sock_path, NULL); > g_assert_cmpint(recv_sock, !=, -1); > + usleep(1000); > > ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + > sizeof(send_buf)); > g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); > close(send_sock[0]); > > ret = qemu_recv(recv_sock, &len, sizeof(len), 0); I would prefer it if we could find a way to fix this race reliably rather than just inserting a delay and hoping it is sufficient. Otherwise the test is likely to be unreliable if run on a heavily loaded or slow machine. thanks -- PMM
On 03/08/2016 05:54 PM, Peter Maydell wrote: > On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote: >> I found the reason for this problem is that >> unix_connect() have not connect to sock_path before iov_send(). >> It need time to establish connection. so can we fix it with usleep() >> like this: >> >> recv_sock = unix_connect(sock_path, NULL); >> g_assert_cmpint(recv_sock, !=, -1); >> + usleep(1000); >> >> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >> sizeof(send_buf)); >> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >> close(send_sock[0]); >> >> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); > > I would prefer it if we could find a way to fix this race > reliably rather than just inserting a delay and hoping it > is sufficient. Otherwise the test is likely to be unreliable > if run on a heavily loaded or slow machine. Yes, but there is no way to know when tcp_chr_accept() is called. Add a event to notify it? Thanks Wen Congyang > > thanks > -- PMM > > >
On 03/09/2016 09:36 AM, Wen Congyang wrote: > On 03/08/2016 05:54 PM, Peter Maydell wrote: >> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote: >>> I found the reason for this problem is that >>> unix_connect() have not connect to sock_path before iov_send(). >>> It need time to establish connection. so can we fix it with usleep() >>> like this: >>> >>> recv_sock = unix_connect(sock_path, NULL); >>> g_assert_cmpint(recv_sock, !=, -1); >>> + usleep(1000); >>> >>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >>> sizeof(send_buf)); >>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >>> close(send_sock[0]); >>> >>> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >> >> I would prefer it if we could find a way to fix this race >> reliably rather than just inserting a delay and hoping it >> is sufficient. Otherwise the test is likely to be unreliable >> if run on a heavily loaded or slow machine. > > Yes, but there is no way to know when tcp_chr_accept() is called. Add a event > to notify it? > > Thanks > Wen Congyang > Hi, Jason, PMM As Congyang said that this is a bug of testcase instead of filter-mirror. Maybe we should re-wrok the testcase, for example - using -chardev pipe instead of -chardev socket, because we are intend to test the packet mirror fuction instead of -chardev socket How about that ? >> >> thanks >> -- PMM >> >> >> > > > > >
On 03/09/2016 12:26 PM, Li Zhijian wrote: > > > On 03/09/2016 09:36 AM, Wen Congyang wrote: >> On 03/08/2016 05:54 PM, Peter Maydell wrote: >>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote: >>>> I found the reason for this problem is that >>>> unix_connect() have not connect to sock_path before iov_send(). >>>> It need time to establish connection. so can we fix it with usleep() >>>> like this: >>>> >>>> recv_sock = unix_connect(sock_path, NULL); >>>> g_assert_cmpint(recv_sock, !=, -1); >>>> + usleep(1000); >>>> >>>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >>>> sizeof(send_buf)); >>>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >>>> close(send_sock[0]); >>>> >>>> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >>> >>> I would prefer it if we could find a way to fix this race >>> reliably rather than just inserting a delay and hoping it >>> is sufficient. Otherwise the test is likely to be unreliable >>> if run on a heavily loaded or slow machine. >> >> Yes, but there is no way to know when tcp_chr_accept() is called. Add a event >> to notify it? >> >> Thanks >> Wen Congyang >> > > Hi, Jason, PMM > As Congyang said that this is a bug of testcase instead of filter-mirror. > Maybe we should re-wrok the testcase, for example > - using -chardev pipe instead of -chardev socket, because we are > intend to test the packet mirror fuction instead of -chardev socket I think it is OK to change it. Thanks Wen Congyang > > How about that ? > > >>> >>> thanks >>> -- PMM >>> >>> >>> >> >> >> >> >> > . >
On 03/08/2016 05:54 PM, Peter Maydell wrote: > On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote: >> I found the reason for this problem is that >> unix_connect() have not connect to sock_path before iov_send(). >> It need time to establish connection. so can we fix it with usleep() >> like this: >> >> recv_sock = unix_connect(sock_path, NULL); >> g_assert_cmpint(recv_sock, !=, -1); >> + usleep(1000); >> >> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >> sizeof(send_buf)); >> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >> close(send_sock[0]); >> >> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); > I would prefer it if we could find a way to fix this race > reliably rather than just inserting a delay and hoping it > is sufficient. Otherwise the test is likely to be unreliable > if run on a heavily loaded or slow machine. > > thanks > -- PMM > +1 To make sure the connected socket to be proceeded before iov_send(), you could use some like qmp("{ 'execute' : 'query-status'}") before iov_send(). With this we are guaranteed that connected is setting to true before iov_send().
On 03/10/2016 10:28 AM, Jason Wang wrote: > > > On 03/08/2016 05:54 PM, Peter Maydell wrote: >> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote: >>> I found the reason for this problem is that >>> unix_connect() have not connect to sock_path before iov_send(). >>> It need time to establish connection. so can we fix it with usleep() >>> like this: >>> >>> recv_sock = unix_connect(sock_path, NULL); >>> g_assert_cmpint(recv_sock, !=, -1); >>> + usleep(1000); >>> >>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >>> sizeof(send_buf)); >>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >>> close(send_sock[0]); >>> >>> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >> I would prefer it if we could find a way to fix this race >> reliably rather than just inserting a delay and hoping it >> is sufficient. Otherwise the test is likely to be unreliable >> if run on a heavily loaded or slow machine. >> >> thanks >> -- PMM >> > > +1 > > To make sure the connected socket to be proceeded before iov_send(), you > could use some like qmp("{ 'execute' : 'query-status'}") before > iov_send(). With this we are guaranteed that connected is setting to > true before iov_send(). > > it seem works, but i don't know. Is this because that both qemu accepting the connection and qmp command are working under *iothread*, so that when the qemu command returns, we can guaranteed the connection is accepted ? Thanks Li Zhijian > > >
On 03/10/2016 11:51 AM, Li Zhijian wrote: > > > On 03/10/2016 10:28 AM, Jason Wang wrote: >> >> >> On 03/08/2016 05:54 PM, Peter Maydell wrote: >>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>> wrote: >>>> I found the reason for this problem is that >>>> unix_connect() have not connect to sock_path before iov_send(). >>>> It need time to establish connection. so can we fix it with usleep() >>>> like this: >>>> >>>> recv_sock = unix_connect(sock_path, NULL); >>>> g_assert_cmpint(recv_sock, !=, -1); >>>> + usleep(1000); >>>> >>>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >>>> sizeof(send_buf)); >>>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >>>> close(send_sock[0]); >>>> >>>> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >>> I would prefer it if we could find a way to fix this race >>> reliably rather than just inserting a delay and hoping it >>> is sufficient. Otherwise the test is likely to be unreliable >>> if run on a heavily loaded or slow machine. >>> >>> thanks >>> -- PMM >>> >> >> +1 >> >> To make sure the connected socket to be proceeded before iov_send(), you >> could use some like qmp("{ 'execute' : 'query-status'}") before >> iov_send(). With this we are guaranteed that connected is setting to >> true before iov_send(). >> >> > > it seem works, but i don't know. > Is this because that both qemu accepting the connection and qmp > command are working under *iothread*, > so that when the qemu command returns, we can guaranteed the > connection is accepted ? I think the problem is the race in main loop between the two handlers. If the socket netdev read handler was proceed before chardev read handler, since connected was false the packet will be dropped silently. After we place whateve a qmp command in the middled, it's guaranteed that all handlers were proceed after qmp command was executed, so we are sure the connected is true when doing iov_send(). > > Thanks > Li Zhijian >> >> >> > > >
On 03/09/2016 12:26 PM, Li Zhijian wrote: > > > On 03/09/2016 09:36 AM, Wen Congyang wrote: >> On 03/08/2016 05:54 PM, Peter Maydell wrote: >>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>> wrote: >>>> I found the reason for this problem is that >>>> unix_connect() have not connect to sock_path before iov_send(). >>>> It need time to establish connection. so can we fix it with usleep() >>>> like this: >>>> >>>> recv_sock = unix_connect(sock_path, NULL); >>>> g_assert_cmpint(recv_sock, !=, -1); >>>> + usleep(1000); >>>> >>>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >>>> sizeof(send_buf)); >>>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >>>> close(send_sock[0]); >>>> >>>> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >>> >>> I would prefer it if we could find a way to fix this race >>> reliably rather than just inserting a delay and hoping it >>> is sufficient. Otherwise the test is likely to be unreliable >>> if run on a heavily loaded or slow machine. >> >> Yes, but there is no way to know when tcp_chr_accept() is called. Add >> a event >> to notify it? >> >> Thanks >> Wen Congyang >> > > Hi, Jason, PMM > As Congyang said that this is a bug of testcase instead of filter-mirror. > Maybe we should re-wrok the testcase, for example > - using -chardev pipe instead of -chardev socket, because we are > intend to test the packet mirror fuction instead of -chardev socket > > How about that ? I still prefer to use socket since it will be more similar to its real usage (e.g COLO). Btw, I haven't investigated deeply, but at least qmp command trick should work. But what's better is to passing the pre-connected fd (e.g the socket created by socketpair()) to chardev, not sure we can do this.
On 03/15/2016 11:07 AM, Jason Wang wrote: > > > On 03/10/2016 11:51 AM, Li Zhijian wrote: >> >> >> On 03/10/2016 10:28 AM, Jason Wang wrote: >>> >>> >>> On 03/08/2016 05:54 PM, Peter Maydell wrote: >>>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>>> wrote: >>>>> I found the reason for this problem is that >>>>> unix_connect() have not connect to sock_path before iov_send(). >>>>> It need time to establish connection. so can we fix it with usleep() >>>>> like this: >>>>> >>>>> recv_sock = unix_connect(sock_path, NULL); >>>>> g_assert_cmpint(recv_sock, !=, -1); >>>>> + usleep(1000); >>>>> >>>>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + >>>>> sizeof(send_buf)); >>>>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); >>>>> close(send_sock[0]); >>>>> >>>>> ret = qemu_recv(recv_sock, &len, sizeof(len), 0); >>>> I would prefer it if we could find a way to fix this race >>>> reliably rather than just inserting a delay and hoping it >>>> is sufficient. Otherwise the test is likely to be unreliable >>>> if run on a heavily loaded or slow machine. >>>> >>>> thanks >>>> -- PMM >>>> >>> >>> +1 >>> >>> To make sure the connected socket to be proceeded before iov_send(), you >>> could use some like qmp("{ 'execute' : 'query-status'}") before >>> iov_send(). With this we are guaranteed that connected is setting to >>> true before iov_send(). >>> >>> >> >> it seem works, but i don't know. >> Is this because that both qemu accepting the connection and qmp >> command are working under *iothread*, >> so that when the qemu command returns, we can guaranteed the >> connection is accepted ? > > I think the problem is the race in main loop between the two handlers. > If the socket netdev read handler was proceed before chardev read > handler, since connected was false the packet will be dropped silently. > After we place whateve a qmp command in the middled, it's guaranteed > that all handlers were proceed after qmp command was executed, so we are > sure the connected is true when doing iov_send(). > Got it, thank for your explain. we will send V9 with '-chardev socket' and add an qmp sync point soon. Thanks Li Zhijian >> >> Thanks >> Li Zhijian >>> >>> >>> >> >> >> > > > > . >
The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f: Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000) are available in the git repository at: https://github.com/jasowang/qemu.git tags/net-pull-request for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e: net: check packet payload length (2016-03-07 10:15:48 +0800) ---------------------------------------------------------------- - a new netfilter implementation: mirror - netfilter could be disabled and enabled through qom-set now - fix netfilter crash when specifiying wrong parameters - rocker switch now can allow user to specifiy world - fix OOB access for ne2000 ---------------------------------------------------------------- Jason Wang (1): net: filter: correctly remove filter from the list during finalization Jiri Pirko (4): rocker: forbid to change world type rocker: return -ENOMEM in case of some world alloc fails rocker: add name field into WorldOps ale let world specify its name rocker: allow user to specify rocker world by property Paolo Bonzini (1): net: simplify net_init_tap_one logic Prasad J Pandit (2): net: ne2000: check ring buffer control registers net: check packet payload length Thomas Huth (1): MAINTAINERS: Add entries for include/net/ files Vincenzo Maffione (1): net: netmap: probe netmap interface for virtio-net header Zhang Chen (2): net/filter-mirror:Add filter-mirror tests/test-filter-mirror:add filter-mirror unit test zhanghailiang (2): filter: Add 'status' property for filter object filter-buffer: Add status_changed callback processing MAINTAINERS | 2 + hw/net/ne2000.c | 4 + hw/net/rocker/rocker.c | 38 ++++++++- hw/net/rocker/rocker_fp.c | 5 ++ hw/net/rocker/rocker_fp.h | 1 + hw/net/rocker/rocker_of_dpa.c | 1 + hw/net/rocker/rocker_world.c | 7 +- hw/net/rocker/rocker_world.h | 1 + include/net/filter.h | 4 + net/Makefile.objs | 1 + net/checksum.c | 10 ++- net/filter-buffer.c | 34 ++++++-- net/filter-mirror.c | 182 ++++++++++++++++++++++++++++++++++++++++++ net/filter.c | 44 +++++++++- net/netmap.c | 59 +++++++++----- net/tap.c | 4 +- qemu-options.hx | 9 ++- tests/.gitignore | 1 + tests/Makefile | 2 + tests/test-filter-mirror.c | 90 +++++++++++++++++++++ vl.c | 3 +- 21 files changed, 460 insertions(+), 42 deletions(-) create mode 100644 net/filter-mirror.c create mode 100644 tests/test-filter-mirror.c