Re: [PATCH] selftests/proc/proc-empty-vm.c: Test for unmapped process

[Shuah Khan <skhan@linuxfoundation.org>]

On 9/30/24 10:09, Siddharth Menon wrote:
> Check if VMsize is 0 to determine whether the process has been unmapped.
> The child process cannot signal the parent that it has unmapped itself,
> as it no longer exists. This includes unmapping the text segment,
> preventing the child from proceeding to the next instruction.

Short log: Add test for unmapped process instead of "Test for"

Also you can include just: selftests/proc in shortlog and mention
the routine you care fixing.

Are you fixing a TODO? Mention that as well and cleanup the
TODO since it is fixed.

> 
> Signed-off-by: Siddharth Menon <simeddon@gmail.com>
> ---
>   tools/testing/selftests/proc/proc-empty-vm.c | 50 ++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/tools/testing/selftests/proc/proc-empty-vm.c b/tools/testing/selftests/proc/proc-empty-vm.c
> index b3f898aab4ab..8ee000b0ddd7 100644
> --- a/tools/testing/selftests/proc/proc-empty-vm.c
> +++ b/tools/testing/selftests/proc/proc-empty-vm.c
> @@ -213,6 +213,53 @@ static void vsyscall(void)
>   }
>   #endif
>   
> +static int test_proc_pid_mem(pid_t pid)
> +{
> +	char buf[4096];
> +	char *line;
> +	int vm_size = -1;
> +
> +	snprintf(buf, sizeof(buf), "/proc/%d/status", pid);
> +	int fd = open(buf, O_RDONLY);
> +
> +	if (fd == -1) {
> +		if (errno == ENOENT) {
> +			// Process does not exist

This isn't the right comment block - please refer to coding style doc
in the repo. Maintain the comment style in this file.

> +			return EXIT_SUCCESS;
> +		}
> +	perror("open /proc/[pid]/status");
> +	return EXIT_FAILURE;
> +	}
> +
> +	ssize_t rv = read(fd, buf, sizeof(buf) - 1);
> +
> +	if (rv == -1) {
> +		perror("read");

Change this
> +		close(fd);
> +		return EXIT_FAILURE;
> +	}
> +	buf[rv] = '\0';
> +
> +	line = strtok(buf, "\n");
> +	while (line != NULL) {
> +		// Check for VmSize

Same here

> +		if (strncmp(line, "VmSize:", 7) == 0) {
> +			sscanf(line, "VmSize: %d", &vm_size);
> +			break;
> +		}
> +		line = strtok(NULL, "\n");
> +	}
> +
> +	close(fd);
> +
> +	// Check if VmSize is 0

Same here

> +	if (vm_size == 0) {
> +		return EXIT_SUCCESS;
> +	}
> +

Did you run checkpatch - you don't need { } for a single line
conditional.

> +	return EXIT_FAILURE;
> +}
> +
>   static int test_proc_pid_maps(pid_t pid)
>   {
>   	char buf[4096];
> @@ -508,6 +555,9 @@ int main(void)
>   		 */
>   		sleep(1);
>   
> +		if (rv == EXIT_SUCCESS) {
> +			rv = test_proc_pid_mem(pid);
> +		}
>   		if (rv == EXIT_SUCCESS) {
>   			rv = test_proc_pid_maps(pid);
>   		}

Also add everybody get_maintianer.pl suggests when you send v2.
You are missing key reviewers and maintainers.

thanks,
-- Shuah