* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all [not found] <00d17d6d-19c9-4431-a3ac-c0f767c533d4@nfschina.com> @ 2025-03-14 10:23 ` Su Hui 2025-03-14 22:13 ` Elizabeth Figura 1 sibling, 0 replies; 8+ messages in thread From: Su Hui @ 2025-03-14 10:23 UTC (permalink / raw) To: Dan Carpenter Cc: zfigura, shuah, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On 2025/3/14 18:14, Su Hui wrote: > On 2025/3/14 17:21, Dan Carpenter wrote: >> On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote: >>> When 'manual=false' and 'signaled=true', then expected value when using >>> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. >>> >>> Signed-off-by: Su Hui<suhui@nfschina.com> >>> --- >>> tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c >>> index 3aad311574c4..bfb6fad653d0 100644 >>> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c >>> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c >>> @@ -968,7 +968,7 @@ TEST(wake_all) >>> auto_event_args.manual = false; >>> auto_event_args.signaled = true; >>> objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); >>> - EXPECT_EQ(0, objs[3]); >>> + EXPECT_LE(0, objs[3]); >> It's kind of weird how these macros put the constant on the left. >> It returns an "fd" on success. So this look reasonable. It probably >> won't return the zero fd so we could probably check EXPECT_LT()? > Agreed, there are about 29 items that can be changed to EXPECT_LT(). > I can send a v2 patchset with this change if there is no more other > suggestions. Sorry for the wrong style of email:(. Su Hui ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all [not found] <00d17d6d-19c9-4431-a3ac-c0f767c533d4@nfschina.com> 2025-03-14 10:23 ` [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all Su Hui @ 2025-03-14 22:13 ` Elizabeth Figura 2025-03-15 9:39 ` Dan Carpenter 1 sibling, 1 reply; 8+ messages in thread From: Elizabeth Figura @ 2025-03-14 22:13 UTC (permalink / raw) To: Dan Carpenter, Su Hui Cc: shuah, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote: > On 2025/3/14 17:21, Dan Carpenter wrote: > > On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote: > >> When 'manual=false' and 'signaled=true', then expected value when using > >> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. > >> > >> Signed-off-by: Su Hui<suhui@nfschina.com> > >> --- > >> tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c > >> index 3aad311574c4..bfb6fad653d0 100644 > >> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c > >> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c > >> @@ -968,7 +968,7 @@ TEST(wake_all) > >> auto_event_args.manual = false; > >> auto_event_args.signaled = true; > >> objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); > >> - EXPECT_EQ(0, objs[3]); > >> + EXPECT_LE(0, objs[3]); > > It's kind of weird how these macros put the constant on the left. > > It returns an "fd" on success. So this look reasonable. It probably > > won't return the zero fd so we could probably check EXPECT_LT()? > Agreed, there are about 29 items that can be changed to EXPECT_LT(). > I can send a v2 patchset with this change if there is no more other > suggestions. I personally think it looks wrong to use EXPECT_LT(), but I'll certainly defer to a higher maintainer on this point. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all 2025-03-14 22:13 ` Elizabeth Figura @ 2025-03-15 9:39 ` Dan Carpenter 2025-03-15 20:29 ` Elizabeth Figura 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2025-03-15 9:39 UTC (permalink / raw) To: Elizabeth Figura Cc: Su Hui, shuah, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On Fri, Mar 14, 2025 at 05:13:50PM -0500, Elizabeth Figura wrote: > On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote: > > On 2025/3/14 17:21, Dan Carpenter wrote: > > > On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote: > > >> When 'manual=false' and 'signaled=true', then expected value when using > > >> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. > > >> > > >> Signed-off-by: Su Hui<suhui@nfschina.com> > > >> --- > > >> tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c > > >> index 3aad311574c4..bfb6fad653d0 100644 > > >> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c > > >> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c > > >> @@ -968,7 +968,7 @@ TEST(wake_all) > > >> auto_event_args.manual = false; > > >> auto_event_args.signaled = true; > > >> objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); > > >> - EXPECT_EQ(0, objs[3]); > > >> + EXPECT_LE(0, objs[3]); > > > It's kind of weird how these macros put the constant on the left. > > > It returns an "fd" on success. So this look reasonable. It probably > > > won't return the zero fd so we could probably check EXPECT_LT()? > > Agreed, there are about 29 items that can be changed to EXPECT_LT(). > > I can send a v2 patchset with this change if there is no more other > > suggestions. > > I personally think it looks wrong to use EXPECT_LT(), but I'll certainly > defer to a higher maintainer on this point. I'm not sure I understand what you are saying. Are you saying that we should allow zero as an expected file descriptor here? I don't have strong feelings about that either way. Putting variables on the right, Yoda speak is. Unnatural is. I did a git grep and the KUNIT_EXPECT_LT() just calls the parameters left and right instead of "expected" and "seen". Expected is wrong for LT because we expect it to be != to the expected value. It's the opposite. We're expecting the unexpected! It would be better to just call them left and right. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all 2025-03-15 9:39 ` Dan Carpenter @ 2025-03-15 20:29 ` Elizabeth Figura 2025-03-17 1:33 ` Su Hui 0 siblings, 1 reply; 8+ messages in thread From: Elizabeth Figura @ 2025-03-15 20:29 UTC (permalink / raw) To: Dan Carpenter Cc: Su Hui, shuah, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On Saturday, 15 March 2025 04:39:46 CDT Dan Carpenter wrote: > On Fri, Mar 14, 2025 at 05:13:50PM -0500, Elizabeth Figura wrote: > > On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote: > > > On 2025/3/14 17:21, Dan Carpenter wrote: > > > > On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote: > > > >> When 'manual=false' and 'signaled=true', then expected value when using > > > >> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. > > > >> > > > >> Signed-off-by: Su Hui<suhui@nfschina.com> > > > >> --- > > > >> tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c > > > >> index 3aad311574c4..bfb6fad653d0 100644 > > > >> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c > > > >> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c > > > >> @@ -968,7 +968,7 @@ TEST(wake_all) > > > >> auto_event_args.manual = false; > > > >> auto_event_args.signaled = true; > > > >> objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); > > > >> - EXPECT_EQ(0, objs[3]); > > > >> + EXPECT_LE(0, objs[3]); > > > > It's kind of weird how these macros put the constant on the left. > > > > It returns an "fd" on success. So this look reasonable. It probably > > > > won't return the zero fd so we could probably check EXPECT_LT()? > > > Agreed, there are about 29 items that can be changed to EXPECT_LT(). > > > I can send a v2 patchset with this change if there is no more other > > > suggestions. > > > > I personally think it looks wrong to use EXPECT_LT(), but I'll certainly > > defer to a higher maintainer on this point. > > I'm not sure I understand what you are saying. Are you saying that we > should allow zero as an expected file descriptor here? I don't have > strong feelings about that either way. Yes, my apologies for the ambiguous wording. That is, EXPECT_LE looks more correct to me than EXPECT_LT per se. > Putting variables on the right, Yoda speak is. Unnatural is. Yes, I certainly agree with this. I wrote it this way in the first place because I was following some other example, I forget which. > I did a git grep and the KUNIT_EXPECT_LT() just calls the parameters > left and right instead of "expected" and "seen". Expected is wrong > for LT because we expect it to be != to the expected value. It's > the opposite. We're expecting the unexpected! It would be better > to just call them left and right. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all 2025-03-15 20:29 ` Elizabeth Figura @ 2025-03-17 1:33 ` Su Hui 0 siblings, 0 replies; 8+ messages in thread From: Su Hui @ 2025-03-17 1:33 UTC (permalink / raw) To: Elizabeth Figura, Dan Carpenter Cc: shuah, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On 2025/3/16 04:29, Elizabeth Figura wrote: > On Saturday, 15 March 2025 04:39:46 CDT Dan Carpenter wrote: >> On Fri, Mar 14, 2025 at 05:13:50PM -0500, Elizabeth Figura wrote: >>> On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote: >>>> On 2025/3/14 17:21, Dan Carpenter wrote: >>>>> On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote: >>>>>> When 'manual=false' and 'signaled=true', then expected value when using >>>>>> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. >>>>>> >>>>>> Signed-off-by: Su Hui<suhui@nfschina.com> >>>>>> --- >>>>>> tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c >>>>>> index 3aad311574c4..bfb6fad653d0 100644 >>>>>> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c >>>>>> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c >>>>>> @@ -968,7 +968,7 @@ TEST(wake_all) >>>>>> auto_event_args.manual = false; >>>>>> auto_event_args.signaled = true; >>>>>> objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); >>>>>> - EXPECT_EQ(0, objs[3]); >>>>>> + EXPECT_LE(0, objs[3]); >>>>> It's kind of weird how these macros put the constant on the left. >>>>> It returns an "fd" on success. So this look reasonable. It probably >>>>> won't return the zero fd so we could probably check EXPECT_LT()? >>>> Agreed, there are about 29 items that can be changed to EXPECT_LT(). >>>> I can send a v2 patchset with this change if there is no more other >>>> suggestions. >>> I personally think it looks wrong to use EXPECT_LT(), but I'll certainly >>> defer to a higher maintainer on this point. >> I'm not sure I understand what you are saying. Are you saying that we >> should allow zero as an expected file descriptor here? I don't have >> strong feelings about that either way. > Yes, my apologies for the ambiguous wording. That is, EXPECT_LE looks more correct to me than EXPECT_LT per se. Got it, I think there is no need for v2 patch that using EXPECT_LT(). Thanks for your feedback. Su Hui ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 0/4] ntsync: some small fixes for doc and selftests @ 2025-03-14 7:14 Su Hui 2025-03-14 7:14 ` [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all Su Hui 0 siblings, 1 reply; 8+ messages in thread From: Su Hui @ 2025-03-14 7:14 UTC (permalink / raw) To: zfigura, corbet, shuah Cc: Su Hui, wine-devel, linux-doc, linux-kernel, linux-kselftest, kernel-janitors There are four small fixes for ntsync test and doc. I divided these into four different patches due to different types of errors. If one patch is better, I can do it too. Su Hui (4): selftests: ntsync: fix the wrong condition in wake_all selftests: ntsync: avoid possible overflow in 32-bit machine selftests: ntsync: update config docs: ntsync: update NTSYNC_IOC_* Documentation/userspace-api/ntsync.rst | 18 +++++++++--------- tools/testing/selftests/drivers/ntsync/config | 2 +- .../testing/selftests/drivers/ntsync/ntsync.c | 6 +++--- 3 files changed, 13 insertions(+), 13 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all 2025-03-14 7:14 [PATCH 0/4] ntsync: some small fixes for doc and selftests Su Hui @ 2025-03-14 7:14 ` Su Hui 2025-03-14 9:21 ` Dan Carpenter 2025-03-14 22:12 ` Elizabeth Figura 0 siblings, 2 replies; 8+ messages in thread From: Su Hui @ 2025-03-14 7:14 UTC (permalink / raw) To: zfigura, shuah Cc: Su Hui, wine-devel, linux-kselftest, linux-kernel, kernel-janitors When 'manual=false' and 'signaled=true', then expected value when using NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. Signed-off-by: Su Hui <suhui@nfschina.com> --- tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c index 3aad311574c4..bfb6fad653d0 100644 --- a/tools/testing/selftests/drivers/ntsync/ntsync.c +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c @@ -968,7 +968,7 @@ TEST(wake_all) auto_event_args.manual = false; auto_event_args.signaled = true; objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); - EXPECT_EQ(0, objs[3]); + EXPECT_LE(0, objs[3]); wait_args.timeout = get_abs_timeout(1000); wait_args.objs = (uintptr_t)objs; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all 2025-03-14 7:14 ` [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all Su Hui @ 2025-03-14 9:21 ` Dan Carpenter 2025-03-14 22:12 ` Elizabeth Figura 1 sibling, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2025-03-14 9:21 UTC (permalink / raw) To: Su Hui Cc: zfigura, shuah, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote: > When 'manual=false' and 'signaled=true', then expected value when using > NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. > > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c > index 3aad311574c4..bfb6fad653d0 100644 > --- a/tools/testing/selftests/drivers/ntsync/ntsync.c > +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c > @@ -968,7 +968,7 @@ TEST(wake_all) > auto_event_args.manual = false; > auto_event_args.signaled = true; > objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); > - EXPECT_EQ(0, objs[3]); > + EXPECT_LE(0, objs[3]); It's kind of weird how these macros put the constant on the left. It returns an "fd" on success. So this look reasonable. It probably won't return the zero fd so we could probably check EXPECT_LT()? regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all 2025-03-14 7:14 ` [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all Su Hui 2025-03-14 9:21 ` Dan Carpenter @ 2025-03-14 22:12 ` Elizabeth Figura 1 sibling, 0 replies; 8+ messages in thread From: Elizabeth Figura @ 2025-03-14 22:12 UTC (permalink / raw) To: shuah, Su Hui Cc: Su Hui, wine-devel, linux-kselftest, linux-kernel, kernel-janitors On Friday, 14 March 2025 02:14:51 CDT Su Hui wrote: > When 'manual=false' and 'signaled=true', then expected value when using > NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error. > > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c > index 3aad311574c4..bfb6fad653d0 100644 > --- a/tools/testing/selftests/drivers/ntsync/ntsync.c > +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c > @@ -968,7 +968,7 @@ TEST(wake_all) > auto_event_args.manual = false; > auto_event_args.signaled = true; > objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args); > - EXPECT_EQ(0, objs[3]); > + EXPECT_LE(0, objs[3]); > > wait_args.timeout = get_abs_timeout(1000); > wait_args.objs = (uintptr_t)objs; > Reviewed-by: Elizabeth Figura <zfigura@codeweavers.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-03-17 1:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <00d17d6d-19c9-4431-a3ac-c0f767c533d4@nfschina.com>
2025-03-14 10:23 ` [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all Su Hui
2025-03-14 22:13 ` Elizabeth Figura
2025-03-15 9:39 ` Dan Carpenter
2025-03-15 20:29 ` Elizabeth Figura
2025-03-17 1:33 ` Su Hui
2025-03-14 7:14 [PATCH 0/4] ntsync: some small fixes for doc and selftests Su Hui
2025-03-14 7:14 ` [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all Su Hui
2025-03-14 9:21 ` Dan Carpenter
2025-03-14 22:12 ` Elizabeth Figura
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox