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

["Rybalchenko, Kirill" <kirill.rybalchenko@intel.com>]

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
>