Re: [PATCH 3/5] fanotify21: Simplify read_pidfd_fdinfo()

On Tue 16-06-26 02:06:27, AnonymeMeow wrote:
> Let callers provide storage for struct pidfd_fdinfo_t instead of
> allocating it in read_pidfd_fdinfo() every time. This removes
> unnecessary allocation/free paths and the NULL checks after
> read_pidfd_fdinfo() and simplifies the code.
> 
> Signed-off-by: AnonymeMeow <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  .../kernel/syscalls/fanotify/fanotify21.c     | 61 ++++++-------------
>  1 file changed, 20 insertions(+), 41 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify21.c b/testcases/kernel/syscalls/fanotify/fanotify21.c
> index bd1a43513..22f9767db 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify21.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify21.c
> @@ -64,7 +64,7 @@ static struct test_case_t {
>  
>  static int fanotify_fd;
>  static char event_buf[BUF_SZ];
> -static struct pidfd_fdinfo_t *self_pidfd_fdinfo;
> +static struct pidfd_fdinfo_t exp_pidfd_fdinfo;
>  
>  static int fd_error_unsupported;
>  
> @@ -73,12 +73,9 @@ static struct tst_clone_args clone_args = {
>  	.exit_signal = SIGCHLD,
>  };
>  
> -static struct pidfd_fdinfo_t *read_pidfd_fdinfo(int pidfd)
> +static void read_pidfd_fdinfo(int pidfd, struct pidfd_fdinfo_t *pidfd_fdinfo)
>  {
>  	char *fdinfo_path;
> -	struct pidfd_fdinfo_t *pidfd_fdinfo;
> -
> -	pidfd_fdinfo = SAFE_MALLOC(sizeof(struct pidfd_fdinfo_t));
>  
>  	SAFE_ASPRINTF(&fdinfo_path, "/proc/self/fdinfo/%d", pidfd);
>  	SAFE_FILE_LINES_SCANF(fdinfo_path, "pos: %d", &pidfd_fdinfo->pos);
> @@ -88,8 +85,6 @@ static struct pidfd_fdinfo_t *read_pidfd_fdinfo(int pidfd)
>  	SAFE_FILE_LINES_SCANF(fdinfo_path, "NSpid: %d", &pidfd_fdinfo->ns_pid);
>  
>  	free(fdinfo_path);
> -
> -	return pidfd_fdinfo;
>  }
>  
>  static void generate_event(void)
> @@ -133,12 +128,9 @@ static void do_setup(void)
>  
>  	pidfd = SAFE_PIDFD_OPEN(getpid(), 0);
>  
> -	self_pidfd_fdinfo = read_pidfd_fdinfo(pidfd);
> -	if (self_pidfd_fdinfo == NULL) {
> -		tst_brk(TBROK,
> -			"pidfd=%d, failed to read pidfd fdinfo",
> -			pidfd);
> -	}
> +	read_pidfd_fdinfo(pidfd, &exp_pidfd_fdinfo);
> +
> +	SAFE_CLOSE(pidfd);
>  }
>  
>  static void do_test(unsigned int num)
> @@ -195,7 +187,7 @@ static void do_test(unsigned int num)
>  	while (i < len) {
>  		struct fanotify_event_metadata *event;
>  		struct fanotify_event_info_pidfd *info;
> -		struct pidfd_fdinfo_t *event_pidfd_fdinfo = NULL;
> +		struct pidfd_fdinfo_t event_pidfd_fdinfo;
>  
>  		event = (struct fanotify_event_metadata *)&event_buf[i];
>  		info = (struct fanotify_event_info_pidfd *)(event + 1);
> @@ -275,39 +267,32 @@ static void do_test(unsigned int num)
>  		 * No pidfd errors occurred, continue with verifying pidfd
>  		 * fdinfo validity.
>  		 */
> -		event_pidfd_fdinfo = read_pidfd_fdinfo(info->pidfd);
> -		if (event_pidfd_fdinfo == NULL) {
> -			tst_brk(TBROK,
> -				"reading fdinfo for pidfd: %d "
> -				"describing pid: %u failed",
> -				info->pidfd,
> -				(unsigned int)event->pid);
> -			goto next_event;
> -		} else if (event_pidfd_fdinfo->pid != event->pid) {
> +		read_pidfd_fdinfo(info->pidfd, &event_pidfd_fdinfo);
> +		if (event_pidfd_fdinfo.pid != event->pid) {
>  			tst_res(TFAIL,
>  				"pidfd provided for incorrect pid "
>  				"(expected pidfd for pid: %u, got pidfd for "
>  				"pid: %u)",
>  				(unsigned int)event->pid,
> -				(unsigned int)event_pidfd_fdinfo->pid);
> +				(unsigned int)event_pidfd_fdinfo.pid);
>  			goto next_event;
> -		} else if (memcmp(event_pidfd_fdinfo, self_pidfd_fdinfo,
> +		} else if (memcmp(&event_pidfd_fdinfo, &exp_pidfd_fdinfo,
>  				  sizeof(struct pidfd_fdinfo_t))) {
>  			tst_res(TFAIL,
>  				"pidfd fdinfo values for self and event differ "
>  				"(expected pos: %d, flags: %x, mnt_id: %d, "
>  				"pid: %d, ns_pid: %d, got pos: %d, "
>  				"flags: %x, mnt_id: %d, pid: %d, ns_pid: %d",
> -				self_pidfd_fdinfo->pos,
> -				self_pidfd_fdinfo->flags,
> -				self_pidfd_fdinfo->mnt_id,
> -				self_pidfd_fdinfo->pid,
> -				self_pidfd_fdinfo->ns_pid,
> -				event_pidfd_fdinfo->pos,
> -				event_pidfd_fdinfo->flags,
> -				event_pidfd_fdinfo->mnt_id,
> -				event_pidfd_fdinfo->pid,
> -				event_pidfd_fdinfo->ns_pid);
> +				exp_pidfd_fdinfo.pos,
> +				exp_pidfd_fdinfo.flags,
> +				exp_pidfd_fdinfo.mnt_id,
> +				exp_pidfd_fdinfo.pid,
> +				exp_pidfd_fdinfo.ns_pid,
> +				event_pidfd_fdinfo.pos,
> +				event_pidfd_fdinfo.flags,
> +				event_pidfd_fdinfo.mnt_id,
> +				event_pidfd_fdinfo.pid,
> +				event_pidfd_fdinfo.ns_pid);
>  			goto next_event;
>  		} else {
>  			tst_res(TPASS,
> @@ -329,9 +314,6 @@ next_event:
>  
>  		if (info && info->pidfd >= 0)
>  			SAFE_CLOSE(info->pidfd);
> -
> -		if (event_pidfd_fdinfo)
> -			free(event_pidfd_fdinfo);
>  	}
>  
>  	if (tc->want_pidfd_err)
> @@ -343,9 +325,6 @@ static void do_cleanup(void)
>  	if (fanotify_fd >= 0)
>  		SAFE_CLOSE(fanotify_fd);
>  
> -	if (self_pidfd_fdinfo)
> -		free(self_pidfd_fdinfo);
> -
>  	/* Unmount the bind mount */
>  	SAFE_UMOUNT(MOUNT_PATH);
>  }
> -- 
> 2.54.0
> 
-- 
Jan Kara <jack-IBi9RG/[email protected]>
SUSE Labs, CR

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp