[PATCH] KVM: selftests: delete some dead code

[PATCH] KVM: selftests: delete some dead code

[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 <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);
-- 
2.26.2

Re: [PATCH] KVM: selftests: delete some dead code

[Paolo Bonzini]

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

Re: [PATCH] KVM: selftests: delete some dead code

[Andrew Jones]

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

Re: [PATCH] KVM: selftests: delete some dead code

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH] KVM: selftests: delete some dead code

[Paolo Bonzini]

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

Re: [PATCH] KVM: selftests: delete some dead code

[Peter Xu]

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!

-- 
Peter Xu