diff mbox series

[V4,6/9] telemetry: refactor mapping between value and array type

Message ID 20221213101512.39919-7-lihuisong@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series telemetry: fix data truncation and conversion error and add hex integer API |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

lihuisong (C) Dec. 13, 2022, 10:15 a.m. UTC 
  Currently, use rte_tel_value_type as index of array to find the
tel_container_types in rte_tel_data_start_array. It's not good
for maintenance.

Fixes: ed1bfad7d384 ("telemetry: add functions for returning callback data")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/telemetry/telemetry_data.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Bruce Richardson Dec. 13, 2022, 4:57 p.m. UTC | #1 
On Tue, Dec 13, 2022 at 06:15:09PM +0800, Huisong Li wrote:
> Currently, use rte_tel_value_type as index of array to find the
> tel_container_types in rte_tel_data_start_array. It's not good
> for maintenance.
> 
> Fixes: ed1bfad7d384 ("telemetry: add functions for returning callback data")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/telemetry/telemetry_data.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 34366ecee3..080d99aec9 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -15,13 +15,29 @@
>  int
>  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
>  {
> -	enum tel_container_types array_types[] = {
> -			RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> -			RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> -			RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
> -			RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> +	struct {
> +		enum rte_tel_value_type value_type;
> +		enum tel_container_types array_type;
> +	} value2array_types_map[] = {
> +		{RTE_TEL_STRING_VAL, RTE_TEL_ARRAY_STRING},
> +		{RTE_TEL_INT_VAL,    RTE_TEL_ARRAY_INT},
> +		{RTE_TEL_U64_VAL,    RTE_TEL_ARRAY_U64},
> +		{RTE_TEL_CONTAINER,  RTE_TEL_ARRAY_CONTAINER},
>  	};
> -	d->type = array_types[type];
> +	int array_types = -1;
> +	uint16_t i;
> +
> +	for (i = 0; i < RTE_DIM(value2array_types_map); i++) {
> +		if (type == value2array_types_map[i].value_type) {
> +			array_types = value2array_types_map[i].array_type;
> +			break;
> +		}
> +	}
> +
While this may need cleanup, I don't think this particular way is the
best method to use, so NAK for this patch for now.

/Bruce
lihuisong (C) Dec. 14, 2022, 2:46 a.m. UTC | #2 
在 2022/12/14 0:57, Bruce Richardson 写道:
> On Tue, Dec 13, 2022 at 06:15:09PM +0800, Huisong Li wrote:
>> Currently, use rte_tel_value_type as index of array to find the
>> tel_container_types in rte_tel_data_start_array. It's not good
>> for maintenance.
>>
>> Fixes: ed1bfad7d384 ("telemetry: add functions for returning callback data")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>   lib/telemetry/telemetry_data.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
>> index 34366ecee3..080d99aec9 100644
>> --- a/lib/telemetry/telemetry_data.c
>> +++ b/lib/telemetry/telemetry_data.c
>> @@ -15,13 +15,29 @@
>>   int
>>   rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
>>   {
>> -	enum tel_container_types array_types[] = {
>> -			RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
>> -			RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
>> -			RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
>> -			RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
>> +	struct {
>> +		enum rte_tel_value_type value_type;
>> +		enum tel_container_types array_type;
>> +	} value2array_types_map[] = {
>> +		{RTE_TEL_STRING_VAL, RTE_TEL_ARRAY_STRING},
>> +		{RTE_TEL_INT_VAL,    RTE_TEL_ARRAY_INT},
>> +		{RTE_TEL_U64_VAL,    RTE_TEL_ARRAY_U64},
>> +		{RTE_TEL_CONTAINER,  RTE_TEL_ARRAY_CONTAINER},
>>   	};
>> -	d->type = array_types[type];
>> +	int array_types = -1;
>> +	uint16_t i;
>> +
>> +	for (i = 0; i < RTE_DIM(value2array_types_map); i++) {
>> +		if (type == value2array_types_map[i].value_type) {
>> +			array_types = value2array_types_map[i].array_type;
>> +			break;
>> +		}
>> +	}
>> +
> While this may need cleanup, I don't think this particular way is the
> best method to use, so NAK for this patch for now.
Can you have some advice to optimize it, Bruce?
>
> /Bruce
> .
diff mbox series

Patch

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 34366ecee3..080d99aec9 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -15,13 +15,29 @@ 
 int
 rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
 {
-	enum tel_container_types array_types[] = {
-			RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
-			RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
-			RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
-			RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
+	struct {
+		enum rte_tel_value_type value_type;
+		enum tel_container_types array_type;
+	} value2array_types_map[] = {
+		{RTE_TEL_STRING_VAL, RTE_TEL_ARRAY_STRING},
+		{RTE_TEL_INT_VAL,    RTE_TEL_ARRAY_INT},
+		{RTE_TEL_U64_VAL,    RTE_TEL_ARRAY_U64},
+		{RTE_TEL_CONTAINER,  RTE_TEL_ARRAY_CONTAINER},
 	};
-	d->type = array_types[type];
+	int array_types = -1;
+	uint16_t i;
+
+	for (i = 0; i < RTE_DIM(value2array_types_map); i++) {
+		if (type == value2array_types_map[i].value_type) {
+			array_types = value2array_types_map[i].array_type;
+			break;
+		}
+	}
+
+	if (array_types == -1)
+		return -EINVAL;
+
+	d->type = array_types;
 	d->data_len = 0;
 	return 0;
 }