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

[Simon Horman <horms@kernel.org>]

On Thu, Sep 28, 2023 at 09:18:08AM -0400, Benjamin Poirier wrote:
> On 2023-09-28 13:21 +0200, Simon Horman wrote:
> > 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
> 
>       ^
> 
> [...]
> 
> > > -		} 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?
> 
> A '\n' is already added when the string is output by pktgen_if_show() so
> if the string above has a trailing '\n', it leads to an empty line in
> the output.
> 
> If my count is correct, before this patch there are 56 cases that output
> to pkt_dev->result/pg_result in pktgen_if_write() and only 3 of them
> include a trailing '\n', arguably by mistake.
> 
> So, I think it's better to remove the empty line than to keep the
> current formatting.

Thanks for the clarification.

I have no further objections, but if the patch is resupn for some other
reason, then it might be worth mentioning this in the patch description.

Reviewed-by: Simon Horman <horms@kernel.org>