diff mbox series

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

Message ID 20230602013358.900637-5-jhubbard@nvidia.com
State New
Headers
Series A minor flurry of selftest/mm fixes |

Commit Message

John Hubbard June 2, 2023, 1:33 a.m. UTC 
  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(-)

Comments

David Hildenbrand June 2, 2023, 10:04 a.m. UTC | #1 
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;
}
Peter Xu June 2, 2023, 3:24 p.m. UTC | #2 
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..
John Hubbard June 2, 2023, 6:52 p.m. UTC | #3 
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,
Peter Xu June 5, 2023, 3:38 p.m. UTC | #4 
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
John Hubbard June 5, 2023, 6:45 p.m. UTC | #5 
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,
diff mbox series

Patch

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