Re: [dpdk-dev] [PATCH v2 1/4] app/testpmd: fix missing verification of port id

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

On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> To set Tx vlan offloads, it is required to stop port firstly. But before
> checking whether the port is stopped, the port id entered by the user
> is not checked for validity. When the port id is illegal, it would lead
> to a segmentation fault since it attempts to access a member of
> non-existent port.
> 
> This patch adds verification of port id in tx vlan offloads.
> 
> Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0a6ed85f3..8377f8401 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -4268,6 +4268,9 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
>  {
>  	struct cmd_tx_vlan_set_result *res = parsed_result;
>  
> +	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> +		return;
> +
>  	if (!port_is_stopped(res->port_id)) {
>  		printf("Please stop port %d first\n", res->port_id);
>  		return;

These functions are wrappers to some testpmd functions, and those
functions already have invalid port check, like 'tx_vlan_set()'

Agree that check should be in these functions, but to prevent the
duplicated checks, what do you think to remove checks from internal
functions? ('tx_vlan_set', 'tx_qinq_set' & 'tx_vlan_reset').