Re: [PATCH v2 1/8] net/bonding: fix typos and whitespace

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Clean up minor typos in comments, strings, and private names.
> - Fix whitespace in log messages and function formatting
>    (new line before open brace).
> - Move closing C++ wrapper to the end of rte_eth_bond_8023ad.h.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   app/test-pmd/cmdline.c                        |  4 ++--
>   app/test/test_link_bonding_mode4.c            | 28 +++++++++++++--------------
>   drivers/net/bonding/eth_bond_8023ad_private.h | 12 ++++++------
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 22 ++++++++++-----------
>   drivers/net/bonding/rte_eth_bond_8023ad.h     | 15 +++++++-------
>   drivers/net/bonding/rte_eth_bond_pmd.c        | 13 ++++++++-----
>   6 files changed, 49 insertions(+), 45 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6e10afe..9fd2c2a 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -630,8 +630,8 @@ static void cmd_help_long_parsed(void *parsed_result,
>   			"set bonding mac_addr (port_id) (address)\n"
>   			"	Set the MAC address of a bonded device.\n\n"
>   
> -			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)"
> -			"	Set Aggregation mode for IEEE802.3AD (mode 4)"
> +			"set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
> +			"	Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"

ack

>   
>   			"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
>   			"	Set the transmit balance policy for bonded device running in balance mode.\n\n"
> diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
> index 351129d..2be86d5 100644
> --- a/app/test/test_link_bonding_mode4.c
> +++ b/app/test/test_link_bonding_mode4.c
> @@ -58,11 +58,11 @@ static const struct rte_ether_addr slave_mac_default = {
>   	{ 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 }
>   };
>   
> -static const struct rte_ether_addr parnter_mac_default = {
> +static const struct rte_ether_addr partner_mac_default = {

ack

<...>

> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index 9b5738a..60db31e 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -15,12 +15,12 @@
>   #include "rte_eth_bond_8023ad.h"
>   
>   #define BOND_MODE_8023AX_UPDATE_TIMEOUT_MS  100
> -/** Maximum number of packets to one slave queued in TX ring. */
> +/** Maximum number of packets to one slave queued in RX ring. */
>   #define BOND_MODE_8023AX_SLAVE_RX_PKTS        3
>   /** Maximum number of LACP packets from one slave queued in TX ring. */
>   #define BOND_MODE_8023AX_SLAVE_TX_PKTS        1
>   /**
> - * Timeouts deffinitions (5.4.4 in 802.1AX documentation).
> + * Timeouts definitions (6.4.4 in 802.1AX documentation).

There are multiple updates in the document reference, section 5 -> 6,
since the old code is consistent about section 5, can it be because
of document version change?
If so should we document the document version to prevent same thing
happen again?
<...>

>   
> -#ifdef __cplusplus
> -}
> -#endif
> -
>   /**
>    * Configure a slave port to start collecting.
>    *
> @@ -331,4 +327,9 @@ rte_eth_bond_8023ad_agg_selection_get(uint16_t port_id);
>   int
>   rte_eth_bond_8023ad_agg_selection_set(uint16_t port_id,
>   		enum rte_bond_8023ad_agg_selection agg_selection);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +

This is not typo or whitespace, it seems we misplaced the cpp block, so this
may result issues for cpp applications using this header, I wonder if we should
have separate patch for fixes?

>   #endif /* RTE_ETH_BOND_8023AD_H_ */
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 84f4900..f6003b0 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -158,7 +158,8 @@ const struct rte_flow_attr flow_attr_8023ad = {
>   
>   int
>   bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
> -		uint16_t slave_port) {
> +		uint16_t slave_port)
> +{

OK to typo fixes, but not sure about the syntax fixes, they corrupt the git
history for little benefit, I think better to fix these when this code is
changed for some other functional reason.
What to you think to drop these changes?

And overall perhaps this change can be split into more patches, that also
helps backporting:
- spelling error, typo, whitespace fixes
- Document reference fix
- ifdef __cplusplus fix