| Message ID | 20221115095941.787250-1-dvyukov@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] NFC: nci: Extend virtual NCI deinit test | expand |
On Tue, 2022-11-15 at 10:59 +0100, Dmitry Vyukov wrote: > Extend the test to check the scenario when NCI core tries to send data > to already closed device to ensure that nothing bad happens. > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Bongsu Jeon <bongsu.jeon@samsung.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org > --- > tools/testing/selftests/nci/nci_dev.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tools/testing/selftests/nci/nci_dev.c b/tools/testing/selftests/nci/nci_dev.c > index 162c41e9bcae8..272958a4ad102 100644 > --- a/tools/testing/selftests/nci/nci_dev.c > +++ b/tools/testing/selftests/nci/nci_dev.c > @@ -888,6 +888,16 @@ TEST_F(NCI, deinit) > &msg); > ASSERT_EQ(rc, 0); > EXPECT_EQ(get_dev_enable_state(&msg), 0); > + > + // Test that operations that normally send packets to the driver > + // don't cause issues when the device is already closed. > + // Note: the send of NFC_CMD_DEV_UP itself still succeeds it's just > + // that the device won't actually be up. > + close(self->virtual_nci_fd); > + self->virtual_nci_fd = -1; I think you need to handle correctly negative value of virtual_nci_fd in FIXTURE_TEARDOWN(NCI), otherwise it should trigger an assert on pthread_join() - read() operation will fail in virtual_deinit*() Cheers, Paolo
On Thu, 17 Nov 2022 at 13:47, Paolo Abeni <pabeni@redhat.com> wrote: > > On Tue, 2022-11-15 at 10:59 +0100, Dmitry Vyukov wrote: > > Extend the test to check the scenario when NCI core tries to send data > > to already closed device to ensure that nothing bad happens. > > > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > > Cc: Bongsu Jeon <bongsu.jeon@samsung.com> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: netdev@vger.kernel.org > > --- > > tools/testing/selftests/nci/nci_dev.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/tools/testing/selftests/nci/nci_dev.c b/tools/testing/selftests/nci/nci_dev.c > > index 162c41e9bcae8..272958a4ad102 100644 > > --- a/tools/testing/selftests/nci/nci_dev.c > > +++ b/tools/testing/selftests/nci/nci_dev.c > > @@ -888,6 +888,16 @@ TEST_F(NCI, deinit) > > &msg); > > ASSERT_EQ(rc, 0); > > EXPECT_EQ(get_dev_enable_state(&msg), 0); > > + > > + // Test that operations that normally send packets to the driver > > + // don't cause issues when the device is already closed. > > + // Note: the send of NFC_CMD_DEV_UP itself still succeeds it's just > > + // that the device won't actually be up. > > + close(self->virtual_nci_fd); > > + self->virtual_nci_fd = -1; > > I think you need to handle correctly negative value of virtual_nci_fd > in FIXTURE_TEARDOWN(NCI), otherwise it should trigger an assert on > pthread_join() - read() operation will fail in virtual_deinit*() Hi Paolo, In this test we also set self->open_state = 0. This will make FIXTURE_TEARDOWN(NCI) skip all of the deinit code. It will still do close(self->virtual_nci_fd) w/o checking the return value. So it will be close(-1), which will return an error, but we won't check it.
On Thu, 2022-11-17 at 14:47 +0100, Dmitry Vyukov wrote: > On Thu, 17 Nov 2022 at 13:47, Paolo Abeni <pabeni@redhat.com> wrote: > > > > On Tue, 2022-11-15 at 10:59 +0100, Dmitry Vyukov wrote: > > > Extend the test to check the scenario when NCI core tries to send data > > > to already closed device to ensure that nothing bad happens. > > > > > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > > > Cc: Bongsu Jeon <bongsu.jeon@samsung.com> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > Cc: Jakub Kicinski <kuba@kernel.org> > > > Cc: netdev@vger.kernel.org > > > --- > > > tools/testing/selftests/nci/nci_dev.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/tools/testing/selftests/nci/nci_dev.c b/tools/testing/selftests/nci/nci_dev.c > > > index 162c41e9bcae8..272958a4ad102 100644 > > > --- a/tools/testing/selftests/nci/nci_dev.c > > > +++ b/tools/testing/selftests/nci/nci_dev.c > > > @@ -888,6 +888,16 @@ TEST_F(NCI, deinit) > > > &msg); > > > ASSERT_EQ(rc, 0); > > > EXPECT_EQ(get_dev_enable_state(&msg), 0); > > > + > > > + // Test that operations that normally send packets to the driver > > > + // don't cause issues when the device is already closed. > > > + // Note: the send of NFC_CMD_DEV_UP itself still succeeds it's just > > > + // that the device won't actually be up. > > > + close(self->virtual_nci_fd); > > > + self->virtual_nci_fd = -1; > > > > I think you need to handle correctly negative value of virtual_nci_fd > > in FIXTURE_TEARDOWN(NCI), otherwise it should trigger an assert on > > pthread_join() - read() operation will fail in virtual_deinit*() > > Hi Paolo, > > In this test we also set self->open_state = 0. This will make > FIXTURE_TEARDOWN(NCI) skip all of the deinit code. It will still do > close(self->virtual_nci_fd) w/o checking the return value. So it will > be close(-1), which will return an error, but we won't check it. Thanks for the pointer. The code looks indeed correct. And sorry for the late nit picking, but I guess it's better to avoid the '//' comment marker and use instead the multi-line ones. Thanks! Paolo
On Thu, 17 Nov 2022 at 15:58, Paolo Abeni <pabeni@redhat.com> wrote: > > > On Tue, 2022-11-15 at 10:59 +0100, Dmitry Vyukov wrote: > > > > Extend the test to check the scenario when NCI core tries to send data > > > > to already closed device to ensure that nothing bad happens. > > > > > > > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > > > > Cc: Bongsu Jeon <bongsu.jeon@samsung.com> > > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > Cc: Jakub Kicinski <kuba@kernel.org> > > > > Cc: netdev@vger.kernel.org > > > > --- > > > > tools/testing/selftests/nci/nci_dev.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/tools/testing/selftests/nci/nci_dev.c b/tools/testing/selftests/nci/nci_dev.c > > > > index 162c41e9bcae8..272958a4ad102 100644 > > > > --- a/tools/testing/selftests/nci/nci_dev.c > > > > +++ b/tools/testing/selftests/nci/nci_dev.c > > > > @@ -888,6 +888,16 @@ TEST_F(NCI, deinit) > > > > &msg); > > > > ASSERT_EQ(rc, 0); > > > > EXPECT_EQ(get_dev_enable_state(&msg), 0); > > > > + > > > > + // Test that operations that normally send packets to the driver > > > > + // don't cause issues when the device is already closed. > > > > + // Note: the send of NFC_CMD_DEV_UP itself still succeeds it's just > > > > + // that the device won't actually be up. > > > > + close(self->virtual_nci_fd); > > > > + self->virtual_nci_fd = -1; > > > > > > I think you need to handle correctly negative value of virtual_nci_fd > > > in FIXTURE_TEARDOWN(NCI), otherwise it should trigger an assert on > > > pthread_join() - read() operation will fail in virtual_deinit*() > > > > Hi Paolo, > > > > In this test we also set self->open_state = 0. This will make > > FIXTURE_TEARDOWN(NCI) skip all of the deinit code. It will still do > > close(self->virtual_nci_fd) w/o checking the return value. So it will > > be close(-1), which will return an error, but we won't check it. > > Thanks for the pointer. The code looks indeed correct. > > And sorry for the late nit picking, but I guess it's better to avoid > the '//' comment marker and use instead the multi-line ones. Right. I am used to a different style and checkpatch did not complain. Sent v2: https://lore.kernel.org/all/20221117162101.1467069-1-dvyukov@google.com/
diff --git a/tools/testing/selftests/nci/nci_dev.c b/tools/testing/selftests/nci/nci_dev.c index 162c41e9bcae8..272958a4ad102 100644 --- a/tools/testing/selftests/nci/nci_dev.c +++ b/tools/testing/selftests/nci/nci_dev.c @@ -888,6 +888,16 @@ TEST_F(NCI, deinit) &msg); ASSERT_EQ(rc, 0); EXPECT_EQ(get_dev_enable_state(&msg), 0); + + // Test that operations that normally send packets to the driver + // don't cause issues when the device is already closed. + // Note: the send of NFC_CMD_DEV_UP itself still succeeds it's just + // that the device won't actually be up. + close(self->virtual_nci_fd); + self->virtual_nci_fd = -1; + rc = send_cmd_with_idx(self->sd, self->fid, self->pid, + NFC_CMD_DEV_UP, self->dev_idex); + EXPECT_EQ(rc, 0); } TEST_HARNESS_MAIN
Extend the test to check the scenario when NCI core tries to send data to already closed device to ensure that nothing bad happens. Signed-off-by: Dmitry Vyukov <dvyukov@google.com> Cc: Bongsu Jeon <bongsu.jeon@samsung.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org --- tools/testing/selftests/nci/nci_dev.c | 10 ++++++++++ 1 file changed, 10 insertions(+) base-commit: f12ed9c04804eec4f1819097a0fd0b4800adac2f prerequisite-patch-id: 214c5357c652cee65ee803d0f45f4b15cfcc9861