| Message ID | 20230920125658.46978-1-liangchen.linux@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 057708a9ca5930d4d9a456c29010f4f90ae760b7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v5,1/2] pktgen: Automate flag enumeration for unknown flag handling | expand |
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 > >
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.
Hello: This series was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Wed, 20 Sep 2023 20:56:57 +0800 you 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> > > [...] Here is the summary with links: - [net-next,v5,1/2] pktgen: Automate flag enumeration for unknown flag handling https://git.kernel.org/netdev/net-next/c/057708a9ca59 - [net-next,v5,2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb https://git.kernel.org/netdev/net-next/c/7c7dd1d64910 You are awesome, thank you!
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>
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'; + } + return count; } if (!strcmp(name, "dst_min") || !strcmp(name, "dst")) {