On Tue, Mar 10, 2026 at 09:09:49AM -0700, Stephen Hemminger wrote: > Convert internal flag fields from int/unsigned int to bool for clarity > and reduced structure size. > > Merge the separate select_phy_mac() and get_infinite_rx_arg() functions > into a single process_bool_flag() handler. The new function also adds > proper validation, rejecting values other than "0", "1", or empty (which > defaults to true). > > Also change num_of_queue from unsigned int to uint16_t since it cannot > exceed RTE_PMD_PCAP_MAX_QUEUES. > > Signed-off-by: Stephen Hemminger <[email protected]>
Acked-by: Bruce Richardson <[email protected]> One minor comment inline below. > --- > drivers/net/pcap/pcap_ethdev.c | 69 +++++++++++++++------------------- > 1 file changed, 30 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c > index 258cffb813..c3270a676c 100644 > --- a/drivers/net/pcap/pcap_ethdev.c > +++ b/drivers/net/pcap/pcap_ethdev.c > @@ -7,6 +7,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <stdbool.h> > #include <time.h> > #include <inttypes.h> > #include <errno.h> > @@ -102,9 +103,9 @@ struct pmd_internals { > char devargs[ETH_PCAP_ARG_MAXLEN]; > struct rte_ether_addr eth_addr; > int if_index; > - int single_iface; > - int phy_mac; > - unsigned int infinite_rx; > + bool single_iface; > + bool phy_mac; > + bool infinite_rx; > }; > > struct pmd_process_private { > @@ -114,25 +115,25 @@ struct pmd_process_private { > }; > > struct pmd_devargs { > - unsigned int num_of_queue; > + uint16_t num_of_queue; > + bool phy_mac; > struct devargs_queue { > pcap_dumper_t *dumper; > pcap_t *pcap; > const char *name; > const char *type; > } queue[RTE_PMD_PCAP_MAX_QUEUES]; > - int phy_mac; > }; > > struct pmd_devargs_all { > struct pmd_devargs rx_queues; > struct pmd_devargs tx_queues; > - int single_iface; > - unsigned int is_tx_pcap; > - unsigned int is_tx_iface; > - unsigned int is_rx_pcap; > - unsigned int is_rx_iface; > - unsigned int infinite_rx; > + bool single_iface; > + bool is_tx_pcap; > + bool is_tx_iface; > + bool is_rx_pcap; > + bool is_rx_iface; > + bool infinite_rx; > }; > > static const char *valid_arguments[] = { > @@ -884,7 +885,7 @@ eth_dev_close(struct rte_eth_dev *dev) > } > } > > - if (internals->phy_mac == 0) > + if (!internals->phy_mac) > /* not dynamically allocated, must not be freed */ > dev->data->mac_addrs = NULL; > > @@ -1209,29 +1210,19 @@ open_tx_iface(const char *key, const char *value, > void *extra_args) > } > > static int > -select_phy_mac(const char *key __rte_unused, const char *value, > - void *extra_args) > +process_bool_flag(const char *key, const char *value, void *extra_args) > { > - if (extra_args) { > - const int phy_mac = atoi(value); > - int *enable_phy_mac = extra_args; > - > - if (phy_mac) > - *enable_phy_mac = 1; > - } > - return 0; > -} > - > -static int > -get_infinite_rx_arg(const char *key __rte_unused, > - const char *value, void *extra_args) > -{ > - if (extra_args) { > - const int infinite_rx = atoi(value); > - int *enable_infinite_rx = extra_args; > - > - if (infinite_rx > 0) > - *enable_infinite_rx = 1; > + bool *flag = extra_args; > + > + if (value == NULL || *value == '\0') { > + *flag = true; /* default with no additional argument */ > + } else if (strcmp(value, "0") == 0) { > + *flag = false; > + } else if (strcmp(value, "1") == 0) { > + *flag = true; > + } else { > + PMD_LOG(ERR, "Invalid '%s' value '%s'", key, value); > + return -1; > } > return 0; > } > @@ -1507,7 +1498,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev) > dumpers.queue[0] = pcaps.queue[0]; > > ret = rte_kvargs_process(kvlist, ETH_PCAP_PHY_MAC_ARG, > - &select_phy_mac, &pcaps.phy_mac); > + &process_bool_flag, &pcaps.phy_mac); > if (ret < 0) > goto free_kvlist; > > @@ -1546,9 +1537,9 @@ pmd_pcap_probe(struct rte_vdev_device *dev) > > if (infinite_rx_arg_cnt == 1) { > ret = rte_kvargs_process(kvlist, > - ETH_PCAP_INFINITE_RX_ARG, > - &get_infinite_rx_arg, > - &devargs_all.infinite_rx); > + ETH_PCAP_INFINITE_RX_ARG, > + &process_bool_flag, > + &devargs_all.infinite_rx); You can shrink the diff by leaving the indentation alone, there is nothing wrong with it! :-) > if (ret < 0) > goto free_kvlist; > PMD_LOG(INFO, "infinite_rx has been %s for %s", > @@ -1698,5 +1689,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_pcap, > ETH_PCAP_RX_IFACE_IN_ARG "=<ifc> " > ETH_PCAP_TX_IFACE_ARG "=<ifc> " > ETH_PCAP_IFACE_ARG "=<ifc> " > - ETH_PCAP_PHY_MAC_ARG "=<int>" > + ETH_PCAP_PHY_MAC_ARG "=<0|1> " > ETH_PCAP_INFINITE_RX_ARG "=<0|1>"); > -- > 2.51.0 >
