[v1] A minor flurry of selftest/mm fixes

[PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

Posted by John Hubbard 2 years, 8 months ago

The stop variable is a char*, so use "\0" when assigning to it, rather
than attempting to assign a character type. This was generating a
warning when compiling with clang.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/mlock2-tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
index 11b2301f3aa3..8ee95077dc25 100644
--- a/tools/testing/selftests/mm/mlock2-tests.c
+++ b/tools/testing/selftests/mm/mlock2-tests.c
@@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
 			printf("cannot parse /proc/self/maps\n");
 			goto out;
 		}
-		stop = '\0';
+		stop = "\0";
 
 		sscanf(line, "%lx", &start);
 		sscanf(end_addr, "%lx", &end);
-- 
2.40.1

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

Posted by David Hildenbrand 2 years, 8 months ago

On 02.06.23 03:33, John Hubbard wrote:
> The stop variable is a char*, so use "\0" when assigning to it, rather
> than attempting to assign a character type. This was generating a
> warning when compiling with clang.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   tools/testing/selftests/mm/mlock2-tests.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
> index 11b2301f3aa3..8ee95077dc25 100644
> --- a/tools/testing/selftests/mm/mlock2-tests.c
> +++ b/tools/testing/selftests/mm/mlock2-tests.c
> @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
>   			printf("cannot parse /proc/self/maps\n");
>   			goto out;
>   		}
> -		stop = '\0';
> +		stop = "\0";
>   
>   		sscanf(line, "%lx", &start);
>   		sscanf(end_addr, "%lx", &end);


I'm probably missing something, but what is the stop variable supposed 
to do here? It's completely unused, no?

if (!strchr(end_addr, ' ')) {
	printf("cannot parse /proc/self/maps\n");
	goto out;
}

-- 
Thanks,

David / dhildenb

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

Posted by Peter Xu 2 years, 8 months ago

On Fri, Jun 02, 2023 at 12:04:57PM +0200, David Hildenbrand wrote:
> On 02.06.23 03:33, John Hubbard wrote:
> > The stop variable is a char*, so use "\0" when assigning to it, rather
> > than attempting to assign a character type. This was generating a
> > warning when compiling with clang.
> > 
> > Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> > ---
> >   tools/testing/selftests/mm/mlock2-tests.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
> > index 11b2301f3aa3..8ee95077dc25 100644
> > --- a/tools/testing/selftests/mm/mlock2-tests.c
> > +++ b/tools/testing/selftests/mm/mlock2-tests.c
> > @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
> >   			printf("cannot parse /proc/self/maps\n");
> >   			goto out;
> >   		}
> > -		stop = '\0';
> > +		stop = "\0";
> >   		sscanf(line, "%lx", &start);
> >   		sscanf(end_addr, "%lx", &end);
> 
> 
> I'm probably missing something, but what is the stop variable supposed to do
> here? It's completely unused, no?
> 
> if (!strchr(end_addr, ' ')) {
> 	printf("cannot parse /proc/self/maps\n");
> 	goto out;
> }

I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot
since the sscanf() just worked..

-- 
Peter Xu

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

Posted by John Hubbard 2 years, 8 months ago

On 6/2/23 08:24, Peter Xu wrote:
> On Fri, Jun 02, 2023 at 12:04:57PM +0200, David Hildenbrand wrote:
>> On 02.06.23 03:33, John Hubbard wrote:
>>> The stop variable is a char*, so use "\0" when assigning to it, rather
>>> than attempting to assign a character type. This was generating a
>>> warning when compiling with clang.
>>>
>>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>>> ---
>>>    tools/testing/selftests/mm/mlock2-tests.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
>>> index 11b2301f3aa3..8ee95077dc25 100644
>>> --- a/tools/testing/selftests/mm/mlock2-tests.c
>>> +++ b/tools/testing/selftests/mm/mlock2-tests.c
>>> @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
>>>    			printf("cannot parse /proc/self/maps\n");
>>>    			goto out;
>>>    		}
>>> -		stop = '\0';
>>> +		stop = "\0";
>>>    		sscanf(line, "%lx", &start);
>>>    		sscanf(end_addr, "%lx", &end);
>>
>>
>> I'm probably missing something, but what is the stop variable supposed to do
>> here? It's completely unused, no?
>>
>> if (!strchr(end_addr, ' ')) {
>> 	printf("cannot parse /proc/self/maps\n");
>> 	goto out;
>> }

Yes it is! I certainly had tunnel vision on that one. I've changed the
patch to simply delete that line, for v2, thanks.

> 
> I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot
> since the sscanf() just worked..
> 

Maybe, yes. Hard to tell the original intent at this point...it might
have been used in an early draft version of the loop that didn't get
posted, perhaps.
thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

Posted by Peter Xu 2 years, 8 months ago

On Fri, Jun 02, 2023 at 11:52:42AM -0700, John Hubbard wrote:
> On 6/2/23 08:24, Peter Xu wrote:
> > On Fri, Jun 02, 2023 at 12:04:57PM +0200, David Hildenbrand wrote:
> > > On 02.06.23 03:33, John Hubbard wrote:
> > > > The stop variable is a char*, so use "\0" when assigning to it, rather
> > > > than attempting to assign a character type. This was generating a
> > > > warning when compiling with clang.
> > > > 
> > > > Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> > > > ---
> > > >    tools/testing/selftests/mm/mlock2-tests.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
> > > > index 11b2301f3aa3..8ee95077dc25 100644
> > > > --- a/tools/testing/selftests/mm/mlock2-tests.c
> > > > +++ b/tools/testing/selftests/mm/mlock2-tests.c
> > > > @@ -50,7 +50,7 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
> > > >    			printf("cannot parse /proc/self/maps\n");
> > > >    			goto out;
> > > >    		}
> > > > -		stop = '\0';
> > > > +		stop = "\0";
> > > >    		sscanf(line, "%lx", &start);
> > > >    		sscanf(end_addr, "%lx", &end);
> > > 
> > > 
> > > I'm probably missing something, but what is the stop variable supposed to do
> > > here? It's completely unused, no?
> > > 
> > > if (!strchr(end_addr, ' ')) {
> > > 	printf("cannot parse /proc/self/maps\n");
> > > 	goto out;
> > > }
> 
> Yes it is! I certainly had tunnel vision on that one. I've changed the
> patch to simply delete that line, for v2, thanks.
> 
> > 
> > I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot
> > since the sscanf() just worked..
> > 
> 
> Maybe, yes. Hard to tell the original intent at this point...it might
> have been used in an early draft version of the loop that didn't get
> posted, perhaps.

I'm pretty sure of it.. see the pattern:

		end_addr = strchr(line, '-');
		if (!end_addr) {
			printf("cannot parse /proc/self/maps\n");
			goto out;
		}
		*end_addr = '\0';

And...

		stop = strchr(end_addr, ' ');
		if (!stop) {
			printf("cannot parse /proc/self/maps\n");
			goto out;
		}
		stop = '\0';    <------------------- only diff here

-- 
Peter Xu

Re: [PATCH 04/12] selftests/mm: fix a char* assignment in mlock2-tests.c

Posted by John Hubbard 2 years, 8 months ago

On 6/5/23 08:38, Peter Xu wrote:
...
>>>> I'm probably missing something, but what is the stop variable supposed to do
>>>> here? It's completely unused, no?
>>>>
>>>> if (!strchr(end_addr, ' ')) {
>>>> 	printf("cannot parse /proc/self/maps\n");
>>>> 	goto out;
>>>> }
>>
>> Yes it is! I certainly had tunnel vision on that one. I've changed the
>> patch to simply delete that line, for v2, thanks.
>>
>>>
>>> I guess it wanted to do "*stop = '\0'" but it just didn't matter a lot
>>> since the sscanf() just worked..
>>>
>>
>> Maybe, yes. Hard to tell the original intent at this point...it might
>> have been used in an early draft version of the loop that didn't get
>> posted, perhaps.
> 
> I'm pretty sure of it.. see the pattern:
> 
> 		end_addr = strchr(line, '-');
> 		if (!end_addr) {
> 			printf("cannot parse /proc/self/maps\n");
> 			goto out;
> 		}
> 		*end_addr = '\0';
> 
> And...
> 
> 		stop = strchr(end_addr, ' ');
> 		if (!stop) {
> 			printf("cannot parse /proc/self/maps\n");
> 			goto out;
> 		}
> 		stop = '\0';    <------------------- only diff here
> 

Yes, and that pattern shows why it wants to be "*stop = '\0';", but
it doesn't show why the author wasted a line of code in the first
place, setting a variable that is not used afterwards.

In other words, changing this to "*stop = '\0';" would make it
look pretty, but it's a non-functional line of code to add. Which
is why I think it should just be deleted at this point.

thanks,
-- 
John Hubbard
NVIDIA