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

[John Hubbard]

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