RE: [PATCH v4 3/3] app/procinfo: support descriptor dump

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----Original Message-----
> From: Dongdong Liu <liudongdong3@huawei.com>
> Subject: [PATCH v4 3/3] app/procinfo: support descriptor dump
> 
> From: "Min Hu (Connor)" <humin29@huawei.com>
> 
> This patch support HW Rx/Tx descriptor dump
> 
> The command is like:
> dpdk-proc-info -a xxxx:xx:xx.x --file-prefix=xxx -- -- --show-rx-descriptor
> queue_id:num


What is num here?  You need to describe about this in commit message.
Is num means number of descriptors? Better use the descriptor word here in num

> 
> dpdk-proc-info -a xxxx:xx:xx.x --file-prefix=xxx -- -- --show-tx-descriptor
> queue_id:num

Same here.

> +/* Enable dump buffer descriptor. */
> +#define MAX_NB_ITEM 2
> +static uint16_t rx_nb_item;
> +static uint16_t tx_nb_item;
> +static uint16_t rx_item_opt[MAX_NB_ITEM]; static uint16_t


Instead of using array to keep queid and number of descriptors info , better have structure with 2 parameters one for queue and one for the number of descriptors.
And You can use  the same structure to declare tx_item.

> +tx_item_opt[MAX_NB_ITEM];
> 

> +		"  --show-rx-descriptor queue_id:num: to display ports Rx
> buffer description by queue id and num\n"
> +		"  --show-tx-descriptor queue_id:num: to display ports Tx
> buffer description by queue id and num\n"

Here also use enough description what is num means or use the another name.

>+				if (ret < MAX_NB_ITEM) {

You can check ret < 0 no need to use MAX_NB_ITEM again.
>+					printf("Rx descriptor param parse error.\n");
>+					return -1;
>+				}

Instead of saying parse error. You might need to give clear reason of failure.
That is invalid queueid:num passed by user or user passed more than one pair.

>+				int ret = parse_descriptor_param(optarg,
>+								 rx_item_opt,
>+								 MAX_NB_ITEM);

Do we need to really pass the MAX_NB_ITEM> I don't think we need you can directly use the macro in the called function.


>+				if (ret < MAX_NB_ITEM) {

You can check ret < 0 no need to use MAX_NB_ITEM again.

>+					printf("Tx descriptor param parse error.\n");
>+					return -1;
>+				}

Same here.

Thanks,
Reshma