| Message ID | 20200605110048.GB978434@mwanda (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: selftests: delete some dead code | expand |
On 05/06/20 13:00, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> > --- > tools/testing/selftests/kvm/demand_paging_test.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > index 360cd3ea4cd67..4eb79621434e6 100644 > --- a/tools/testing/selftests/kvm/demand_paging_test.c > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > @@ -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
On Fri, Jun 05, 2020 at 01:16:59PM +0200, Paolo Bonzini wrote: > On 05/06/20 13:00, Dan Carpenter wrote: > > 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 <dan.carpenter@oracle.com> > > --- > > tools/testing/selftests/kvm/demand_paging_test.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > > index 360cd3ea4cd67..4eb79621434e6 100644 > > --- a/tools/testing/selftests/kvm/demand_paging_test.c > > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > > @@ -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. 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
On Fri, Jun 05, 2020 at 01:53:16PM +0200, Andrew Jones wrote: > On Fri, Jun 05, 2020 at 01:16:59PM +0200, Paolo Bonzini wrote: > > On 05/06/20 13:00, Dan Carpenter wrote: > > > 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 <dan.carpenter@oracle.com> > > > --- > > > tools/testing/selftests/kvm/demand_paging_test.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > > > index 360cd3ea4cd67..4eb79621434e6 100644 > > > --- a/tools/testing/selftests/kvm/demand_paging_test.c > > > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > > > @@ -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. Could I ask why? > > 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,
On 05/06/20 14:48, Peter Xu wrote: >>> 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
On Fri, Jun 05, 2020 at 03:26:39PM +0200, Paolo Bonzini wrote: > On 05/06/20 14:48, Peter Xu wrote: > >>> 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 Fair enough, and a good reading material. :) Thanks!
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 360cd3ea4cd67..4eb79621434e6 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -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 "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 <dan.carpenter@oracle.com> --- tools/testing/selftests/kvm/demand_paging_test.c | 2 -- 1 file changed, 2 deletions(-)