* [PATCH v3] examples/qos_meter: fix unchecked return value
@ 2016-05-09 8:38 Slawomir Mrozowicz
2016-05-10 17:41 ` Dumitrescu, Cristian
0 siblings, 1 reply; 7+ messages in thread
From: Slawomir Mrozowicz @ 2016-05-09 8:38 UTC (permalink / raw)
To: cristian.dumitrescu; +Cc: dev, jasvinder.singh, Slawomir Mrozowicz
Fix issue reported by Coverity.
Coverity ID 30693: Unchecked return value
check_return: Calling rte_meter_srtcm_config without checking return value.
Fixes: e6541fdec8b2 ("meter: initial import")
Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
---
examples/qos_meter/main.c | 15 ++++++++++-----
examples/qos_meter/main.h | 2 +-
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
index b968b00..7c69606 100644
--- a/examples/qos_meter/main.c
+++ b/examples/qos_meter/main.c
@@ -133,14 +133,17 @@ struct rte_meter_trtcm_params app_trtcm_params[] = {
FLOW_METER app_flows[APP_FLOWS_MAX];
-static void
+static int
app_configure_flow_table(void)
{
uint32_t i, j;
+ int ret = 0;
- for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) % RTE_DIM(PARAMS)){
- FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
- }
+ for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
+ i ++, j = (j + 1) % RTE_DIM(PARAMS))
+ ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
+
+ return ret;
}
static inline void
@@ -381,7 +384,9 @@ main(int argc, char **argv)
rte_eth_promiscuous_enable(port_tx);
/* App configuration */
- app_configure_flow_table();
+ ret = app_configure_flow_table();
+ if (ret < 0)
+ rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
/* Launch per-lcore init on every lcore */
rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
diff --git a/examples/qos_meter/main.h b/examples/qos_meter/main.h
index 530bf69..54867dc 100644
--- a/examples/qos_meter/main.h
+++ b/examples/qos_meter/main.h
@@ -51,7 +51,7 @@ enum policer_action policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] =
#if APP_MODE == APP_MODE_FWD
#define FUNC_METER(a,b,c,d) color, flow_id=flow_id, pkt_len=pkt_len, time=time
-#define FUNC_CONFIG(a,b)
+#define FUNC_CONFIG(a, b) 0
#define PARAMS app_srtcm_params
#define FLOW_METER int
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] examples/qos_meter: fix unchecked return value
2016-05-09 8:38 [PATCH v3] examples/qos_meter: fix unchecked return value Slawomir Mrozowicz
@ 2016-05-10 17:41 ` Dumitrescu, Cristian
2016-05-11 9:14 ` Mrozowicz, SlawomirX
0 siblings, 1 reply; 7+ messages in thread
From: Dumitrescu, Cristian @ 2016-05-10 17:41 UTC (permalink / raw)
To: Mrozowicz, SlawomirX; +Cc: dev, Singh, Jasvinder
> -----Original Message-----
> From: Mrozowicz, SlawomirX
> Sent: Monday, May 9, 2016 9:38 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
>
> Fix issue reported by Coverity.
>
> Coverity ID 30693: Unchecked return value
> check_return: Calling rte_meter_srtcm_config without checking return
> value.
>
> Fixes: e6541fdec8b2 ("meter: initial import")
>
> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
> examples/qos_meter/main.c | 15 ++++++++++-----
> examples/qos_meter/main.h | 2 +-
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
> index b968b00..7c69606 100644
> --- a/examples/qos_meter/main.c
> +++ b/examples/qos_meter/main.c
> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
> app_trtcm_params[] = {
>
> FLOW_METER app_flows[APP_FLOWS_MAX];
>
> -static void
> +static int
> app_configure_flow_table(void)
> {
> uint32_t i, j;
> + int ret = 0;
>
> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
> RTE_DIM(PARAMS)){
> - FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> - }
> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
> + ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> +
> + return ret;
> }
This is only returns the configuration status for the last flow and leaves undetected an error for any other flow. Why not check the status for each flow and return an error on first occurrence?
For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
>
> static inline void
> @@ -381,7 +384,9 @@ main(int argc, char **argv)
> rte_eth_promiscuous_enable(port_tx);
>
> /* App configuration */
> - app_configure_flow_table();
> + ret = app_configure_flow_table();
> + if (ret < 0)
> + rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
>
> /* Launch per-lcore init on every lcore */
> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
> diff --git a/examples/qos_meter/main.h b/examples/qos_meter/main.h
> index 530bf69..54867dc 100644
> --- a/examples/qos_meter/main.h
> +++ b/examples/qos_meter/main.h
> @@ -51,7 +51,7 @@ enum policer_action
> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] =
> #if APP_MODE == APP_MODE_FWD
>
> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id, pkt_len=pkt_len,
> time=time
> -#define FUNC_CONFIG(a,b)
> +#define FUNC_CONFIG(a, b) 0
> #define PARAMS app_srtcm_params
> #define FLOW_METER int
>
> --
> 1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] examples/qos_meter: fix unchecked return value
2016-05-10 17:41 ` Dumitrescu, Cristian
@ 2016-05-11 9:14 ` Mrozowicz, SlawomirX
2016-05-11 17:57 ` Dumitrescu, Cristian
0 siblings, 1 reply; 7+ messages in thread
From: Mrozowicz, SlawomirX @ 2016-05-11 9:14 UTC (permalink / raw)
To: Dumitrescu, Cristian; +Cc: dev, Singh, Jasvinder
>-----Original Message-----
>From: Dumitrescu, Cristian
>Sent: Tuesday, May 10, 2016 7:42 PM
>To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
>Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
>> -----Original Message-----
>> From: Mrozowicz, SlawomirX
>> Sent: Monday, May 9, 2016 9:38 AM
>> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
>> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
>>
>> Fix issue reported by Coverity.
>>
>> Coverity ID 30693: Unchecked return value
>> check_return: Calling rte_meter_srtcm_config without checking return
>> value.
>>
>> Fixes: e6541fdec8b2 ("meter: initial import")
>>
>> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
>> ---
>> examples/qos_meter/main.c | 15 ++++++++++-----
>> examples/qos_meter/main.h | 2 +-
>> 2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
>> index b968b00..7c69606 100644
>> --- a/examples/qos_meter/main.c
>> +++ b/examples/qos_meter/main.c
>> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
>app_trtcm_params[]
>> = {
>>
>> FLOW_METER app_flows[APP_FLOWS_MAX];
>>
>> -static void
>> +static int
>> app_configure_flow_table(void)
>> {
>> uint32_t i, j;
>> + int ret = 0;
>>
>> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
>> RTE_DIM(PARAMS)){
>> - FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
>> - }
>> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
>> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
>> + ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
>> +
>> + return ret;
>> }
>
>This is only returns the configuration status for the last flow and leaves
>undetected an error for any other flow. Why not check the status for each
>flow and return an error on first occurrence?
>For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
>
This code check status of the function FUNC_CONFIG for each flow and return an error on first occurrence. Rest of functions FUNC_CONFIG are not called. See terminate condition of the loop.
>>
>> static inline void
>> @@ -381,7 +384,9 @@ main(int argc, char **argv)
>> rte_eth_promiscuous_enable(port_tx);
>>
>> /* App configuration */
>> - app_configure_flow_table();
>> + ret = app_configure_flow_table();
>> + if (ret < 0)
>> + rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
>>
>> /* Launch per-lcore init on every lcore */
>> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER); diff --
>git
>> a/examples/qos_meter/main.h b/examples/qos_meter/main.h index
>> 530bf69..54867dc 100644
>> --- a/examples/qos_meter/main.h
>> +++ b/examples/qos_meter/main.h
>> @@ -51,7 +51,7 @@ enum policer_action
>> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] = #if
>APP_MODE
>> == APP_MODE_FWD
>>
>> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id, pkt_len=pkt_len,
>> time=time -#define FUNC_CONFIG(a,b)
>> +#define FUNC_CONFIG(a, b) 0
>> #define PARAMS app_srtcm_params
>> #define FLOW_METER int
>>
>> --
>> 1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] examples/qos_meter: fix unchecked return value
2016-05-11 9:14 ` Mrozowicz, SlawomirX
@ 2016-05-11 17:57 ` Dumitrescu, Cristian
2016-05-12 7:06 ` Mrozowicz, SlawomirX
0 siblings, 1 reply; 7+ messages in thread
From: Dumitrescu, Cristian @ 2016-05-11 17:57 UTC (permalink / raw)
To: Mrozowicz, SlawomirX; +Cc: dev, Singh, Jasvinder
> -----Original Message-----
> From: Mrozowicz, SlawomirX
> Sent: Wednesday, May 11, 2016 10:15 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
> >-----Original Message-----
> >From: Dumitrescu, Cristian
> >Sent: Tuesday, May 10, 2016 7:42 PM
> >To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> >Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
> >
> >
> >
> >> -----Original Message-----
> >> From: Mrozowicz, SlawomirX
> >> Sent: Monday, May 9, 2016 9:38 AM
> >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> >> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> >> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> >> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
> >>
> >> Fix issue reported by Coverity.
> >>
> >> Coverity ID 30693: Unchecked return value
> >> check_return: Calling rte_meter_srtcm_config without checking return
> >> value.
> >>
> >> Fixes: e6541fdec8b2 ("meter: initial import")
> >>
> >> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> >> ---
> >> examples/qos_meter/main.c | 15 ++++++++++-----
> >> examples/qos_meter/main.h | 2 +-
> >> 2 files changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
> >> index b968b00..7c69606 100644
> >> --- a/examples/qos_meter/main.c
> >> +++ b/examples/qos_meter/main.c
> >> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
> >app_trtcm_params[]
> >> = {
> >>
> >> FLOW_METER app_flows[APP_FLOWS_MAX];
> >>
> >> -static void
> >> +static int
> >> app_configure_flow_table(void)
> >> {
> >> uint32_t i, j;
> >> + int ret = 0;
> >>
> >> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
> >> RTE_DIM(PARAMS)){
> >> - FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> >> - }
> >> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
> >> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
> >> + ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> >> +
> >> + return ret;
> >> }
> >
> >This is only returns the configuration status for the last flow and leaves
> >undetected an error for any other flow. Why not check the status for each
> >flow and return an error on first occurrence?
> >For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
> >
>
> This code check status of the function FUNC_CONFIG for each flow and
> return an error on first occurrence. Rest of functions FUNC_CONFIG are not
> called. See terminate condition of the loop.
>
Where is the status of FUNC_CONFIG checked exactly? I cannot see any check in your code. I can only see returning the status code for the last call of this function in the for loop. I was expecting a check such as: if (ret) return ret.
> >>
> >> static inline void
> >> @@ -381,7 +384,9 @@ main(int argc, char **argv)
> >> rte_eth_promiscuous_enable(port_tx);
> >>
> >> /* App configuration */
> >> - app_configure_flow_table();
> >> + ret = app_configure_flow_table();
> >> + if (ret < 0)
> >> + rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
> >>
> >> /* Launch per-lcore init on every lcore */
> >> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER); diff -
> -
> >git
> >> a/examples/qos_meter/main.h b/examples/qos_meter/main.h index
> >> 530bf69..54867dc 100644
> >> --- a/examples/qos_meter/main.h
> >> +++ b/examples/qos_meter/main.h
> >> @@ -51,7 +51,7 @@ enum policer_action
> >> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] = #if
> >APP_MODE
> >> == APP_MODE_FWD
> >>
> >> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id, pkt_len=pkt_len,
> >> time=time -#define FUNC_CONFIG(a,b)
> >> +#define FUNC_CONFIG(a, b) 0
> >> #define PARAMS app_srtcm_params
> >> #define FLOW_METER int
> >>
> >> --
> >> 1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] examples/qos_meter: fix unchecked return value
2016-05-11 17:57 ` Dumitrescu, Cristian
@ 2016-05-12 7:06 ` Mrozowicz, SlawomirX
2016-05-12 9:40 ` Dumitrescu, Cristian
2016-05-12 9:49 ` Dumitrescu, Cristian
0 siblings, 2 replies; 7+ messages in thread
From: Mrozowicz, SlawomirX @ 2016-05-12 7:06 UTC (permalink / raw)
To: Dumitrescu, Cristian; +Cc: dev, Singh, Jasvinder
>-----Original Message-----
>From: Dumitrescu, Cristian
>Sent: Wednesday, May 11, 2016 7:57 PM
>To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
>Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
>> -----Original Message-----
>> From: Mrozowicz, SlawomirX
>> Sent: Wednesday, May 11, 2016 10:15 AM
>> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
>> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>>
>>
>>
>> >-----Original Message-----
>> >From: Dumitrescu, Cristian
>> >Sent: Tuesday, May 10, 2016 7:42 PM
>> >To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>> >Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
>> >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return
>> >value
>> >
>> >
>> >
>> >> -----Original Message-----
>> >> From: Mrozowicz, SlawomirX
>> >> Sent: Monday, May 9, 2016 9:38 AM
>> >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> >> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
>> >> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>> >> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
>> >>
>> >> Fix issue reported by Coverity.
>> >>
>> >> Coverity ID 30693: Unchecked return value
>> >> check_return: Calling rte_meter_srtcm_config without checking
>> >> return value.
>> >>
>> >> Fixes: e6541fdec8b2 ("meter: initial import")
>> >>
>> >> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
>> >> ---
>> >> examples/qos_meter/main.c | 15 ++++++++++-----
>> >> examples/qos_meter/main.h | 2 +-
>> >> 2 files changed, 11 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
>> >> index b968b00..7c69606 100644
>> >> --- a/examples/qos_meter/main.c
>> >> +++ b/examples/qos_meter/main.c
>> >> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
>> >app_trtcm_params[]
>> >> = {
>> >>
>> >> FLOW_METER app_flows[APP_FLOWS_MAX];
>> >>
>> >> -static void
>> >> +static int
>> >> app_configure_flow_table(void)
>> >> {
>> >> uint32_t i, j;
>> >> + int ret = 0;
>> >>
>> >> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
>> >> RTE_DIM(PARAMS)){
>> >> - FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
>> >> - }
>> >> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
>> >> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
>> >> + ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
>> >> +
>> >> + return ret;
>> >> }
>> >
>> >This is only returns the configuration status for the last flow and
>> >leaves undetected an error for any other flow. Why not check the
>> >status for each flow and return an error on first occurrence?
>> >For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
>> >
>>
>> This code check status of the function FUNC_CONFIG for each flow and
>> return an error on first occurrence. Rest of functions FUNC_CONFIG are
>> not called. See terminate condition of the loop.
>>
>
>Where is the status of FUNC_CONFIG checked exactly? I cannot see any check
>in your code. I can only see returning the status code for the last call of this
>function in the for loop. I was expecting a check such as: if (ret) return ret.
>
Look at the loop terminate conditions:
i < APP_FLOWS_MAX && ret == 0
Program terminate the loop if the ret variable is differ then zero.
It means that program terminate if the last status of FUNC_CONFIG is an error.
>> >>
>> >> static inline void
>> >> @@ -381,7 +384,9 @@ main(int argc, char **argv)
>> >> rte_eth_promiscuous_enable(port_tx);
>> >>
>> >> /* App configuration */
>> >> - app_configure_flow_table();
>> >> + ret = app_configure_flow_table();
>> >> + if (ret < 0)
>> >> + rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
>> >>
>> >> /* Launch per-lcore init on every lcore */
>> >> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER); diff -
>> -
>> >git
>> >> a/examples/qos_meter/main.h b/examples/qos_meter/main.h index
>> >> 530bf69..54867dc 100644
>> >> --- a/examples/qos_meter/main.h
>> >> +++ b/examples/qos_meter/main.h
>> >> @@ -51,7 +51,7 @@ enum policer_action
>> >> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] = #if
>> >APP_MODE
>> >> == APP_MODE_FWD
>> >>
>> >> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id,
>> >> pkt_len=pkt_len, time=time -#define FUNC_CONFIG(a,b)
>> >> +#define FUNC_CONFIG(a, b) 0
>> >> #define PARAMS app_srtcm_params
>> >> #define FLOW_METER int
>> >>
>> >> --
>> >> 1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] examples/qos_meter: fix unchecked return value
2016-05-12 7:06 ` Mrozowicz, SlawomirX
@ 2016-05-12 9:40 ` Dumitrescu, Cristian
2016-05-12 9:49 ` Dumitrescu, Cristian
1 sibling, 0 replies; 7+ messages in thread
From: Dumitrescu, Cristian @ 2016-05-12 9:40 UTC (permalink / raw)
To: Mrozowicz, SlawomirX; +Cc: dev, Singh, Jasvinder
> -----Original Message-----
> From: Mrozowicz, SlawomirX
> Sent: Thursday, May 12, 2016 8:06 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
> >-----Original Message-----
> >From: Dumitrescu, Cristian
> >Sent: Wednesday, May 11, 2016 7:57 PM
> >To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> >Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
> >
> >
> >
> >> -----Original Message-----
> >> From: Mrozowicz, SlawomirX
> >> Sent: Wednesday, May 11, 2016 10:15 AM
> >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> >> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> >> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Dumitrescu, Cristian
> >> >Sent: Tuesday, May 10, 2016 7:42 PM
> >> >To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> >> >Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> >> >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return
> >> >value
> >> >
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Mrozowicz, SlawomirX
> >> >> Sent: Monday, May 9, 2016 9:38 AM
> >> >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> >> >> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> >> >> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> >> >> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
> >> >>
> >> >> Fix issue reported by Coverity.
> >> >>
> >> >> Coverity ID 30693: Unchecked return value
> >> >> check_return: Calling rte_meter_srtcm_config without checking
> >> >> return value.
> >> >>
> >> >> Fixes: e6541fdec8b2 ("meter: initial import")
> >> >>
> >> >> Signed-off-by: Slawomir Mrozowicz
> <slawomirx.mrozowicz@intel.com>
> >> >> ---
> >> >> examples/qos_meter/main.c | 15 ++++++++++-----
> >> >> examples/qos_meter/main.h | 2 +-
> >> >> 2 files changed, 11 insertions(+), 6 deletions(-)
> >> >>
> >> >> diff --git a/examples/qos_meter/main.c
> b/examples/qos_meter/main.c
> >> >> index b968b00..7c69606 100644
> >> >> --- a/examples/qos_meter/main.c
> >> >> +++ b/examples/qos_meter/main.c
> >> >> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
> >> >app_trtcm_params[]
> >> >> = {
> >> >>
> >> >> FLOW_METER app_flows[APP_FLOWS_MAX];
> >> >>
> >> >> -static void
> >> >> +static int
> >> >> app_configure_flow_table(void)
> >> >> {
> >> >> uint32_t i, j;
> >> >> + int ret = 0;
> >> >>
> >> >> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
> >> >> RTE_DIM(PARAMS)){
> >> >> - FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> >> >> - }
> >> >> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
> >> >> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
> >> >> + ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> >> >> +
> >> >> + return ret;
> >> >> }
> >> >
> >> >This is only returns the configuration status for the last flow and
> >> >leaves undetected an error for any other flow. Why not check the
> >> >status for each flow and return an error on first occurrence?
> >> >For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
> >> >
> >>
> >> This code check status of the function FUNC_CONFIG for each flow and
> >> return an error on first occurrence. Rest of functions FUNC_CONFIG are
> >> not called. See terminate condition of the loop.
> >>
> >
> >Where is the status of FUNC_CONFIG checked exactly? I cannot see any
> check
> >in your code. I can only see returning the status code for the last call of this
> >function in the for loop. I was expecting a check such as: if (ret) return ret.
> >
>
> Look at the loop terminate conditions:
> i < APP_FLOWS_MAX && ret == 0
> Program terminate the loop if the ret variable is differ then zero.
> It means that program terminate if the last status of FUNC_CONFIG is an
> error.
Yes, you're right, my bad, sorry.
>
> >> >>
> >> >> static inline void
> >> >> @@ -381,7 +384,9 @@ main(int argc, char **argv)
> >> >> rte_eth_promiscuous_enable(port_tx);
> >> >>
> >> >> /* App configuration */
> >> >> - app_configure_flow_table();
> >> >> + ret = app_configure_flow_table();
> >> >> + if (ret < 0)
> >> >> + rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
> >> >>
> >> >> /* Launch per-lcore init on every lcore */
> >> >> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER); diff -
> >> -
> >> >git
> >> >> a/examples/qos_meter/main.h b/examples/qos_meter/main.h index
> >> >> 530bf69..54867dc 100644
> >> >> --- a/examples/qos_meter/main.h
> >> >> +++ b/examples/qos_meter/main.h
> >> >> @@ -51,7 +51,7 @@ enum policer_action
> >> >> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] = #if
> >> >APP_MODE
> >> >> == APP_MODE_FWD
> >> >>
> >> >> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id,
> >> >> pkt_len=pkt_len, time=time -#define FUNC_CONFIG(a,b)
> >> >> +#define FUNC_CONFIG(a, b) 0
> >> >> #define PARAMS app_srtcm_params
> >> >> #define FLOW_METER int
> >> >>
> >> >> --
> >> >> 1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] examples/qos_meter: fix unchecked return value
2016-05-12 7:06 ` Mrozowicz, SlawomirX
2016-05-12 9:40 ` Dumitrescu, Cristian
@ 2016-05-12 9:49 ` Dumitrescu, Cristian
1 sibling, 0 replies; 7+ messages in thread
From: Dumitrescu, Cristian @ 2016-05-12 9:49 UTC (permalink / raw)
To: Mrozowicz, SlawomirX; +Cc: dev, Singh, Jasvinder
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Thursday, May 12, 2016 10:41 AM
> To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
> > -----Original Message-----
> > From: Mrozowicz, SlawomirX
> > Sent: Thursday, May 12, 2016 8:06 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> > Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
> >
> >
> >
> > >-----Original Message-----
> > >From: Dumitrescu, Cristian
> > >Sent: Wednesday, May 11, 2016 7:57 PM
> > >To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> > >Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> > >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: Mrozowicz, SlawomirX
> > >> Sent: Wednesday, May 11, 2016 10:15 AM
> > >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > >> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> > >> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return
> value
> > >>
> > >>
> > >>
> > >> >-----Original Message-----
> > >> >From: Dumitrescu, Cristian
> > >> >Sent: Tuesday, May 10, 2016 7:42 PM
> > >> >To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> > >> >Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
> > >> >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return
> > >> >value
> > >> >
> > >> >
> > >> >
> > >> >> -----Original Message-----
> > >> >> From: Mrozowicz, SlawomirX
> > >> >> Sent: Monday, May 9, 2016 9:38 AM
> > >> >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > >> >> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> > >> >> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> > >> >> Subject: [PATCH v3] examples/qos_meter: fix unchecked return
> value
> > >> >>
> > >> >> Fix issue reported by Coverity.
> > >> >>
> > >> >> Coverity ID 30693: Unchecked return value
> > >> >> check_return: Calling rte_meter_srtcm_config without checking
> > >> >> return value.
> > >> >>
> > >> >> Fixes: e6541fdec8b2 ("meter: initial import")
> > >> >>
> > >> >> Signed-off-by: Slawomir Mrozowicz
> > <slawomirx.mrozowicz@intel.com>
> > >> >> ---
> > >> >> examples/qos_meter/main.c | 15 ++++++++++-----
> > >> >> examples/qos_meter/main.h | 2 +-
> > >> >> 2 files changed, 11 insertions(+), 6 deletions(-)
> > >> >>
> > >> >> diff --git a/examples/qos_meter/main.c
> > b/examples/qos_meter/main.c
> > >> >> index b968b00..7c69606 100644
> > >> >> --- a/examples/qos_meter/main.c
> > >> >> +++ b/examples/qos_meter/main.c
> > >> >> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
> > >> >app_trtcm_params[]
> > >> >> = {
> > >> >>
> > >> >> FLOW_METER app_flows[APP_FLOWS_MAX];
> > >> >>
> > >> >> -static void
> > >> >> +static int
> > >> >> app_configure_flow_table(void)
> > >> >> {
> > >> >> uint32_t i, j;
> > >> >> + int ret = 0;
> > >> >>
> > >> >> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
> > >> >> RTE_DIM(PARAMS)){
> > >> >> - FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> > >> >> - }
> > >> >> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
> > >> >> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
> > >> >> + ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
> > >> >> +
> > >> >> + return ret;
> > >> >> }
> > >> >
> > >> >This is only returns the configuration status for the last flow and
> > >> >leaves undetected an error for any other flow. Why not check the
> > >> >status for each flow and return an error on first occurrence?
> > >> >For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
> > >> >
> > >>
> > >> This code check status of the function FUNC_CONFIG for each flow and
> > >> return an error on first occurrence. Rest of functions FUNC_CONFIG are
> > >> not called. See terminate condition of the loop.
> > >>
> > >
> > >Where is the status of FUNC_CONFIG checked exactly? I cannot see any
> > check
> > >in your code. I can only see returning the status code for the last call of
> this
> > >function in the for loop. I was expecting a check such as: if (ret) return ret.
> > >
> >
> > Look at the loop terminate conditions:
> > i < APP_FLOWS_MAX && ret == 0
> > Program terminate the loop if the ret variable is differ then zero.
> > It means that program terminate if the last status of FUNC_CONFIG is an
> > error.
>
> Yes, you're right, my bad, sorry.
>
Actually, although the logic is correct, personally I find this code very cryptic / hard to read and more difficult to extend later if need be, can we please make it a bit more readable:
for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) % RTE_DIM(PARAMS)) {
ret = FUNC_CONFIG(&app_flows[i], &PARAMS[j]);
if (ret)
return ret;
}
return ret;
Thanks, Slawomir!
> >
> > >> >>
> > >> >> static inline void
> > >> >> @@ -381,7 +384,9 @@ main(int argc, char **argv)
> > >> >> rte_eth_promiscuous_enable(port_tx);
> > >> >>
> > >> >> /* App configuration */
> > >> >> - app_configure_flow_table();
> > >> >> + ret = app_configure_flow_table();
> > >> >> + if (ret < 0)
> > >> >> + rte_exit(EXIT_FAILURE, "Invalid configure flow
> table\n");
> > >> >>
> > >> >> /* Launch per-lcore init on every lcore */
> > >> >> rte_eal_mp_remote_launch(main_loop, NULL,
> CALL_MASTER); diff -
> > >> -
> > >> >git
> > >> >> a/examples/qos_meter/main.h b/examples/qos_meter/main.h
> index
> > >> >> 530bf69..54867dc 100644
> > >> >> --- a/examples/qos_meter/main.h
> > >> >> +++ b/examples/qos_meter/main.h
> > >> >> @@ -51,7 +51,7 @@ enum policer_action
> > >> >> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] =
> #if
> > >> >APP_MODE
> > >> >> == APP_MODE_FWD
> > >> >>
> > >> >> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id,
> > >> >> pkt_len=pkt_len, time=time -#define FUNC_CONFIG(a,b)
> > >> >> +#define FUNC_CONFIG(a, b) 0
> > >> >> #define PARAMS app_srtcm_params
> > >> >> #define FLOW_METER int
> > >> >>
> > >> >> --
> > >> >> 1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-12 9:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-09 8:38 [PATCH v3] examples/qos_meter: fix unchecked return value Slawomir Mrozowicz
2016-05-10 17:41 ` Dumitrescu, Cristian
2016-05-11 9:14 ` Mrozowicz, SlawomirX
2016-05-11 17:57 ` Dumitrescu, Cristian
2016-05-12 7:06 ` Mrozowicz, SlawomirX
2016-05-12 9:40 ` Dumitrescu, Cristian
2016-05-12 9:49 ` Dumitrescu, Cristian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.