Re: [PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[Simon Horman <horms@kernel.org>]

On Wed, Sep 20, 2023 at 08:56:57PM +0800, Liang Chen wrote:
> When specifying an unknown flag, it will print all available flags.
> Currently, these flags are provided as fixed strings, which requires
> manual updates when flags change. Replacing it with automated flag
> enumeration.
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---
>  Changes from v3:
> - check "n == IPSEC_SHIFT" instead of string comparison
> - use snprintf and check that the result does not overrun pkg_dev->result[]
> - avoid double '\n' at the end
> - move "return" in the OK case to remove "else" and decrease indent
> ---
>  net/core/pktgen.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f56b8d697014..48306a101fd9 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1318,9 +1318,10 @@ static ssize_t pktgen_if_write(struct file *file,
>  		return count;
>  	}
>  	if (!strcmp(name, "flag")) {
> +		bool disable = false;
>  		__u32 flag;
>  		char f[32];
> -		bool disable = false;
> +		char *end;
>  
>  		memset(f, 0, 32);
>  		len = strn_len(&user_buffer[i], sizeof(f) - 1);
> @@ -1332,28 +1333,33 @@ static ssize_t pktgen_if_write(struct file *file,
>  		i += len;
>  
>  		flag = pktgen_read_flag(f, &disable);
> -
>  		if (flag) {
>  			if (disable)
>  				pkt_dev->flags &= ~flag;
>  			else
>  				pkt_dev->flags |= flag;
> -		} else {
> -			sprintf(pg_result,
> -				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> -				f,
> -				"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
> -				"MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, "
> -				"MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, "
> -				"QUEUE_MAP_RND, QUEUE_MAP_CPU, UDPCSUM, "
> -				"NO_TIMESTAMP, "
> -#ifdef CONFIG_XFRM
> -				"IPSEC, "
> -#endif
> -				"NODE_ALLOC\n");
> +
> +			sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
>  			return count;
>  		}
> -		sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> +
> +		/* Unknown flag */
> +		end = pkt_dev->result + sizeof(pkt_dev->result);
> +		pg_result += sprintf(pg_result,
> +			"Flag -:%s:- unknown\n"
> +			"Available flags, (prepend ! to un-set flag):\n", f);
> +
> +		for (int n = 0; n < NR_PKT_FLAGS && pg_result < end; n++) {
> +			if (!IS_ENABLED(CONFIG_XFRM) && n == IPSEC_SHIFT)
> +				continue;
> +			pg_result += snprintf(pg_result, end - pg_result,
> +					      "%s, ", pkt_flag_names[n]);
> +		}
> +		if (!WARN_ON_ONCE(pg_result >= end)) {
> +			/* Remove the comma and whitespace at the end */
> +			*(pg_result - 2) = '\0';

Hi Liang Chen,

Should the string have a trailing '\n' in keeping with the current formatting?

> +		}
> +
>  		return count;
>  	}
>  	if (!strcmp(name, "dst_min") || !strcmp(name, "dst")) {
> -- 
> 2.31.1
> 
>