[PATCH 2/6] Extend printfd() to collect extra information about fd

[Masatake YAMATO]

From: "Dmitry V. Levin" <ldv at altlinux.org>
Subject: Re: [PATCH 2/6] Extend printfd() to collect extra information about fd
Date: Fri, 11 Mar 2022 02:05:47 +0300

> On Thu, Mar 10, 2022 at 09:01:02PM +0900, Masatake YAMATO wrote:
> [...]
>> @@ -726,18 +735,36 @@ print_quoted_string_in_angle_brackets(const char *str)
>>  }
>>  
>>  void
>> -printfd_pid(struct tcb *tcp, pid_t pid, int fd)
>> +printfd_pid_filling_info(struct tcb *tcp, pid_t pid, int fd, struct fd_info **fd_info)
>>  {
>>  	PRINT_VAL_D(fd);
>>  
>> +	bool should_print = false;
>>  	char path[PATH_MAX + 1];
>> -	if (pid > 0 && !number_set_array_is_empty(decode_fd_set, 0)
>> +	if (pid > 0
>> +	    && ((should_print = !number_set_array_is_empty(decode_fd_set, 0)) || fd_info)
>>  	    && getfdpath_pid(pid, fd, path, sizeof(path)) >= 0) {
>> +		/*
>> +		 * If FD_INFO is given, FT_INFO must be filled.
>> +		 *
>> +		 * <A> If the information of fd will be printed (SHOULD_PRINT),
>> +		 *     a helper function, printdev(), for printing also fills
>> +		 *     FD_INFO as side effect.
>> +		 * <B> If the information of fd will not be printed (!SHOULD_PRINT),
>> +		 *     we must force to call the helper function for
>> +		 *     filling FD_INFO. However, nothing should be
>> +		 *     printed. Calling disable_tprint/enable_tprint
>> +		 *     is for disabling the output stream.
>> +		 */
>> +		void *tprint_state = NULL;
>> +		if (!should_print)
>> +			tprint_state = disable_tprint();
>> +
>>  		if (is_number_in_set(DECODE_FD_SOCKET, decode_fd_set) &&
>>  		    printsocket(tcp, fd, path))
>>  			goto printed;
>> -		if (is_number_in_set(DECODE_FD_DEV, decode_fd_set) &&
>> -		    printdev(tcp, fd, path))
>> +		if ((fd_info || is_number_in_set(DECODE_FD_DEV, decode_fd_set)) &&
>> +		    printdev(tcp, fd, path, fd_info))
>>  			goto printed;
>>  		if (is_number_in_set(DECODE_FD_PIDFD, decode_fd_set) &&
>>  		    printpidfd(pid, fd, path))
>> @@ -745,6 +772,8 @@ printfd_pid(struct tcb *tcp, pid_t pid, int fd)
>>  		if (is_number_in_set(DECODE_FD_PATH, decode_fd_set))
>>  			print_quoted_string_in_angle_brackets(path);
>>  printed:	;
>> +		if (!should_print)
>> +			enable_tprint(tprint_state);
>>  	}
> 
> I'm sorry, but I don't like this disable_tprint/enable_tprint approach.
> It's not just suboptimal because of wasted work in this function,
> it also looks ugly.
> If all you need from this function is printdev's side effect,
> wouldn't it be better to split printdev instead?

Thank you for reviewing. I will revise the changes.

Masatake YAMATO

> 
> -- 
> ldv
> -- 
> Strace-devel mailing list
> Strace-devel at lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel
>