[dpdk-dev,v4] app/testpmd:add bond type description

  In function cmd_show_bonding_config_parsed() used number represent
the bond type,in order more detailed,add bond type description
otherwise we may confused about the number type.
And also,the primary port just use in mode active backup and tlb,
so,when the mode is active backup or tlb show the primary port info
may be more appropriate.

Signed-off-by: Rongqiang XIE <xie.rongqiang@zte.com.cn>
---
 app/test-pmd/cmdline.c                       | 26 +++++++++++-------
 drivers/net/bonding/rte_eth_bond.h           | 15 +++++++++++
 drivers/net/bonding/rte_eth_bond_api.c       | 40 ++++++++++++++++++++++++++++
 drivers/net/bonding/rte_eth_bond_version.map |  1 +
 4 files changed, 73 insertions(+), 9 deletions(-)
  

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rongqiang XIE
> Sent: Tuesday 29 August 2017 06:09
> To: Wu, Jingjing <jingjing.wu@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Rongqiang XIE <xie.rongqiang@zte.com.cn>
> Subject: [dpdk-dev] [PATCH v4] app/testpmd:add bond type description
> 
> In function cmd_show_bonding_config_parsed() used number represent
> the bond type,in order more detailed,add bond type description otherwise
> we may confused about the number type.
> And also,the primary port just use in mode active backup and tlb, so,when
> the mode is active backup or tlb show the primary port info may be more
> appropriate.
> 
> Signed-off-by: Rongqiang XIE <xie.rongqiang@zte.com.cn>
> ---
>  app/test-pmd/cmdline.c                       | 26 +++++++++++-------
>  drivers/net/bonding/rte_eth_bond.h           | 15 +++++++++++
>  drivers/net/bonding/rte_eth_bond_api.c       | 40
> ++++++++++++++++++++++++++++
>  drivers/net/bonding/rte_eth_bond_version.map |  1 +
>  4 files changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> cd8c358..8395e02 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -4593,6 +4593,7 @@ static void cmd_show_bonding_config_parsed(void
> *parsed_result,  {
>  	struct cmd_show_bonding_config_result *res = parsed_result;
>  	int bonding_mode, agg_mode;
> +	char bonding_str[BONDING_MODE_STRING_LEN];
>  	uint8_t slaves[RTE_MAX_ETHPORTS];
>  	int num_slaves, num_active_slaves;
>  	int primary_id;
> @@ -4600,13 +4601,17 @@ static void
> cmd_show_bonding_config_parsed(void *parsed_result,
>  	portid_t port_id = res->port_id;
> 
>  	/* Display the bonding mode.*/
> +	if (!rte_eth_bond_mode_string_get(port_id, bonding_str)) {
> +		printf("\tFailed to get bonding mode string for port = %d\n",
> port_id);
> +		return;
> +	}
> +	printf("\tBonding mode: %s\n", bonding_str);
> +
>  	bonding_mode = rte_eth_bond_mode_get(port_id);
>  	if (bonding_mode < 0) {
>  		printf("\tFailed to get bonding mode for port = %d\n",
> port_id);
>  		return;
> -	} else
> -		printf("\tBonding mode: %d\n", bonding_mode);
> -
> +	}
>  	if (bonding_mode == BONDING_MODE_BALANCE) {
>  		int balance_xmit_policy;
> 
> @@ -4685,13 +4690,16 @@ static void
> cmd_show_bonding_config_parsed(void *parsed_result,
>  		printf("\tActive Slaves: []\n");
> 
>  	}
> -
> -	primary_id = rte_eth_bond_primary_get(port_id);
> -	if (primary_id < 0) {
> -		printf("\tFailed to get primary slave for port = %d\n",
> port_id);
> -		return;
> -	} else
> +	if (bonding_mode == BONDING_MODE_ACTIVE_BACKUP ||
> +		bonding_mode == BONDING_MODE_TLB){
> +		primary_id = rte_eth_bond_primary_get(port_id);
> +		if (primary_id < 0) {
> +			printf("\tFailed to get primary slave for port = %d\n",
> port_id);
> +			return;
> +		}
>  		printf("\tPrimary: [%d]\n", primary_id);
> +	}
> +	return;
> 
>  }
> 
> diff --git a/drivers/net/bonding/rte_eth_bond.h
> b/drivers/net/bonding/rte_eth_bond.h
> index 8efbf07..c25293a 100644
> --- a/drivers/net/bonding/rte_eth_bond.h
> +++ b/drivers/net/bonding/rte_eth_bond.h
> @@ -117,6 +117,9 @@
>  #define BALANCE_XMIT_POLICY_LAYER34		(2)
>  /**< Layer 3+4 (IP Addresses + UDP Ports) transmit load balancing */
> 
> +/* Max length size for bond mode string */
> +#define BONDING_MODE_STRING_LEN      (30)
> +
>  /**
>   * Create a bonded rte_eth_dev device
>   *
> @@ -189,6 +192,18 @@
>  rte_eth_bond_mode_get(uint8_t bonded_port_id);
> 
>  /**
> + * Get link bonding mode string of bonded device
> + *
> + * @param bonded_port_id	Port ID of bonded device.
> + *
> + * @param mode          mode string
> + * @return
> + *	link bonding mode on success, negative value otherwise
> + */
> +int
> +rte_eth_bond_mode_string_get(uint8_t bonded_port_id, char *mode);
> +
> +/**
>   * Set slave rte_eth_dev as primary slave of bonded device
>   *
>   * @param bonded_port_id	Port ID of bonded device.
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c
> b/drivers/net/bonding/rte_eth_bond_api.c
> index de1d9e0..c55b90f 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -510,6 +510,46 @@
>  }
> 
>  int
> +rte_eth_bond_mode_string_get(uint8_t bonded_port_id, char *mode) {
> +	struct bond_dev_private *internals;
> +
> +	if (valid_bonded_port_id(bonded_port_id) != 0)
> +		return -1;
> +
> +	internals = rte_eth_devices[bonded_port_id].data->dev_private;
> +
> +	switch (internals->mode) {
> +	case BONDING_MODE_ROUND_ROBIN:
> +			memcpy(mode, "round-robin",
> BONDING_MODE_STRING_LEN);

This code might generate segmentation fault, as memcpy will request memory outside source buffer.
Would you consider using strncpy instead?
The same for all subsequent memcpys in this function.

> +			break;
> +	case BONDING_MODE_ACTIVE_BACKUP:
> +			memcpy(mode, "active-backup",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	case BONDING_MODE_BALANCE:
> +			memcpy(mode, "link-aggregation",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	case BONDING_MODE_BROADCAST:
> +			memcpy(mode, "broadcast",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	case BONDING_MODE_8023AD:
> +			memcpy(mode, "link-aggregation-802.3ad",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	case BONDING_MODE_TLB:
> +			memcpy(mode, "transmit-load-balancing",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	case BONDING_MODE_ALB:
> +			memcpy(mode, "adaptive-load-balancing",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	default:
> +			memcpy(mode, "unknown-mode",
> BONDING_MODE_STRING_LEN);
> +			break;
> +	}
> +
> +	return 0;
> +}
> +
> +int
>  rte_eth_bond_primary_set(uint8_t bonded_port_id, uint8_t slave_port_id)
> {
>  	struct bond_dev_private *internals;
> diff --git a/drivers/net/bonding/rte_eth_bond_version.map
> b/drivers/net/bonding/rte_eth_bond_version.map
> index 0f4e847..c08f60e 100644
> --- a/drivers/net/bonding/rte_eth_bond_version.map
> +++ b/drivers/net/bonding/rte_eth_bond_version.map
> @@ -53,6 +53,7 @@ DPDK_17.08 {
>          rte_eth_bond_8023ad_agg_selection_set;
>          rte_eth_bond_8023ad_conf_get;
>          rte_eth_bond_8023ad_setup;
> +        rte_eth_bond_mode_string_get;
> 
> 
>  } DPDK_16.07;
> --
> 1.8.3.1
>
  

On 8/28/2017 10:08 PM, Rongqiang XIE wrote:
> In function cmd_show_bonding_config_parsed() used number represent
> the bond type,in order more detailed,add bond type description
> otherwise we may confused about the number type.
> And also,the primary port just use in mode active backup and tlb,
> so,when the mode is active backup or tlb show the primary port info
> may be more appropriate.

Hi Rongqiang,

Sorry for the delay, this is waiting maintainer Ack for a while.

I hope you are still interested in patch, there is already outstanding comment
from Krill and I would also ask to split this into two patches, the bonding mode
string one and bonding mode check (backup || tlb) one.

Thanks,
ferruh

> 
> Signed-off-by: Rongqiang XIE <xie.rongqiang@zte.com.cn>

<...>