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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> > > > ---
> > > > 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
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