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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux