KVM: selftests: delete some dead code

PATCH In review Dan Carpenter 4 participants 6 messages Jun 5, 2020

Subsystems (3)

KVM Kselftest Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2020-06-05 11:00 UTC #0 lore raw

PATCH 1/1 KVM: selftests: delete some dead code

The "uffd_delay" variable is unsigned so it's always going to be >= 0.

Fixes 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay")

Signed-off-by Dan Carpenter <[email protected]>

---
 tools/testing/selftests/kvm/demand_paging_test.c | 2 --
 1 file changed, 2 deletions(-)

tools/testing/selftests/kvm/demand_paging_test.c +0 -2

index 360cd3ea4cd67..4eb79621434e6 100644
@@ -615,8 +615,6 @@ int main(int argc, char *argv[])
 			break;
 		case 'd':
 			uffd_delay = strtoul(optarg, NULL, 0);
-			TEST_ASSERT(uffd_delay >= 0,
-				    "A negative UFFD delay is not supported.");
 			break;
 		case 'b':
 			vcpu_memory_bytes = parse_size(optarg);

Signature · 1 line

2.26.2

Paolo Bonzini

maintainer

2020-06-05 11:16 UTC #1 lore raw

Quoted 14 lines · Dan Carpenter

The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay")
Signed-off-by Dan Carpenter <[email protected]>
---
 tools/testing/selftests/kvm/demand_paging_test.c | 2 --
 1 file changed, 2 deletions(-)
tools/testing/selftests/kvm/demand_paging_test.c +0 -0
index 360cd3ea4cd67..4eb79621434e6 100644

tools/testing/selftests/kvm/demand_paging_test.c int main(int argc, char *argv[]) · line 620

Quoted 8 lines

tools/testing/selftests/kvm/demand_paging_test.c +0 -2

@@ -615,8 +615,6 @@ int main(int argc, char *argv[])
 			break;
 		case 'd':
 			uffd_delay = strtoul(optarg, NULL, 0);
-			TEST_ASSERT(uffd_delay >= 0,
-				    "A negative UFFD delay is not supported.");
 			break;
 		case 'b':
 			vcpu_memory_bytes = parse_size(optarg);

The bug is that strtoul is "impossible" to use correctly. The right fix would be to have a replacement for strtoul.

Paolo

Andrew Jones

2020-06-05 11:53 UTC #2 lore raw

Quoted 18 lines · Paolo Bonzini

Quoted 17 lines · Dan Carpenter
The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay")
Signed-off-by Dan Carpenter <[email protected]>
---
 tools/testing/selftests/kvm/demand_paging_test.c | 2 --
 1 file changed, 2 deletions(-)
tools/testing/selftests/kvm/demand_paging_test.c +0 -1
index 360cd3ea4cd67..4eb79621434e6 100644
@@ -615,8 +615,6 @@ int main(int argc, char *argv[])
 			break;
 		case 'd':
 			uffd_delay = strtoul(optarg, NULL, 0);
-			TEST_ASSERT(uffd_delay >= 0,

Quoted 8 lines

tools/testing/selftests/kvm/demand_paging_test.c int main(int argc, char *argv[]) · line 620
Quoted 5 lines
tools/testing/selftests/kvm/demand_paging_test.c +0 -1
-				    "A negative UFFD delay is not supported.");
 			break;
 		case 'b':
 			vcpu_memory_bytes = parse_size(optarg);
The bug is that strtoul is "impossible" to use correctly. The right fix would be to have a replacement for strtoul.

The test needs an upper limit. It obviously doesn't make sense to ever want a ULONG_MAX usec delay. What's the maximum number of usecs we should allow?

Thanks,
drew

Peter Xu

2020-06-05 12:48 UTC #3 lore raw

Quoted 10 lines · Andrew Jones

Quoted 9 lines · Paolo Bonzini
Quoted 8 lines · Dan Carpenter
The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay")
Signed-off-by Dan Carpenter <[email protected]>
---
 tools/testing/selftests/kvm/demand_paging_test.c | 2 --
 1 file changed, 2 deletions(-)

Quoted 16 lines

Quoted 8 lines
Quoted 8 lines
tools/testing/selftests/kvm/demand_paging_test.c +0 -0
index 360cd3ea4cd67..4eb79621434e6 100644
@@ -615,8 +615,6 @@ int main(int argc, char *argv[])
 			break;
 		case 'd':
 			uffd_delay = strtoul(optarg, NULL, 0);
Quoted 8 lines
tools/testing/selftests/kvm/demand_paging_test.c int main(int argc, char *argv[]) · line 620
Quoted 6 lines
tools/testing/selftests/kvm/demand_paging_test.c +0 -2
-			TEST_ASSERT(uffd_delay >= 0,
-				    "A negative UFFD delay is not supported.");
 			break;
 		case 'b':
 			vcpu_memory_bytes = parse_size(optarg);
The bug is that strtoul is "impossible" to use correctly.

Could I ask why?

Quoted 6 lines

Quoted 2 lines
The right fix
would be to have a replacement for strtoul.
The test needs an upper limit. It obviously doesn't make sense to ever want a ULONG_MAX usec delay. What's the maximum number of usecs we should allow?

Maybe this test can also be used to emulate a hang-forever kvm mmu fault due to some reason we wanted, by specifying an extremely large value here? From that POV, seems still ok to even keep it unbound as a test...

Thanks,

Signature · 1 line

Peter Xu

Paolo Bonzini

maintainer

2020-06-05 13:26 UTC #4 lore raw

Quoted 2 lines · Peter Xu

Quoted 1 line
Quoted 1 line
The bug is that strtoul is "impossible" to use correctly.
Could I ask why?

To see see how annoying the situation is, check out utils/cutils.c in QEMU; basically, it is very hard to do error handling. From the man page:

Since strtoul() can legitimately return 0 or ULONG_MAX (ULLONG_MAX for strtoull()) on both success and failure, the calling program should set errno to 0 before the call, and then determine if an error occurred by checking whether errno has a nonzero value after the call.

and of course no one wants to write code for that every time they have to parse a number.

In addition, if the string is empty it returns 0, and of endptr is NULL it will accept something like "123abc" and return 123.

So it is not literally impossible, but it is a poorly-designed interface which is a major source of bugs. On Rusty's API design levels[1][2], I would put it at 3 if I'm feeling generous ("Read the documentation and you'll get it right"), and at -4 to -7 ("The obvious use is wrong") if it's been a bad day.

Therefore it's quite common to have a wrapper like

    int my_strtoul(char *p, char **endptr, unsigned long *result);

The wrapper will:

- check that the string is not empty

- always return 0 or -1 because of the by-reference output argument "result"

- take care of checking that the entire input string was parsed, for example by rejecting partial parsing of the string if endptr = NULL.

This version gets a solid 7 ("The obvious use is probably the correct one"); possibly even 8 ("The compiler will warn if you get it wrong") because the output argument gives you better protection against overflow.

Regarding overflow, there is a strtol but not a strtoi, so you need to have a temporary long and do range checking manually. Again, you will most likely make mistakes if you use strtol, while my_strtol will merely make it annoying but it should be obvious that you're getting it wrong.

Paolo

[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
[2] https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html

Peter Xu

2020-06-05 17:39 UTC #5 lore raw

Quoted 47 lines · Paolo Bonzini

Quoted 2 lines · Peter Xu
Quoted 1 line
Quoted 1 line
The bug is that strtoul is "impossible" to use correctly.
Could I ask why?
To see see how annoying the situation is, check out utils/cutils.c in QEMU; basically, it is very hard to do error handling. From the man page:
Since strtoul() can legitimately return 0 or ULONG_MAX (ULLONG_MAX for strtoull()) on both success and failure, the calling program should set errno to 0 before the call, and then determine if an error occurred by checking whether errno has a nonzero value after the call.
and of course no one wants to write code for that every time they have to parse a number.
In addition, if the string is empty it returns 0, and of endptr is NULL it will accept something like "123abc" and return 123.
So it is not literally impossible, but it is a poorly-designed interface which is a major source of bugs. On Rusty's API design levels[1][2], I would put it at 3 if I'm feeling generous ("Read the documentation and you'll get it right"), and at -4 to -7 ("The obvious use is wrong") if it's been a bad day.
Therefore it's quite common to have a wrapper like
    int my_strtoul(char *p, char **endptr, unsigned long *result);
The wrapper will:
- check that the string is not empty
- always return 0 or -1 because of the by-reference output argument "result"
- take care of checking that the entire input string was parsed, for example by rejecting partial parsing of the string if endptr = NULL.
This version gets a solid 7 ("The obvious use is probably the correct one"); possibly even 8 ("The compiler will warn if you get it wrong") because the output argument gives you better protection against overflow.
Regarding overflow, there is a strtol but not a strtoi, so you need to have a temporary long and do range checking manually. Again, you will most likely make mistakes if you use strtol, while my_strtol will merely make it annoying but it should be obvious that you're getting it wrong.

Quoted 4 lines

Paolo
[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
[2] https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html

Fair enough, and a good reading material. :)

Thanks!

Signature · 1 line

Peter Xu