| Message ID | 1462783091-2538-1-git-send-email-slawomirx.mrozowicz@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 2628B5A83; Mon, 9 May 2016 10:35:09 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 2E0F55A69 for <dev@dpdk.org>; Mon, 9 May 2016 10:35:07 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 09 May 2016 01:35:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,600,1455004800"; d="scan'208";a="971615278" Received: from gklab-246-018.igk.intel.com (HELO stargo) ([10.217.246.18]) by orsmga002.jf.intel.com with SMTP; 09 May 2016 01:35:04 -0700 Received: by stargo (sSMTP sendmail emulation); Mon, 09 May 2016 10:38:15 +0200 From: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com> To: cristian.dumitrescu@intel.com Cc: dev@dpdk.org, jasvinder.singh@intel.com, Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com> Date: Mon, 9 May 2016 10:38:11 +0200 Message-Id: <1462783091-2538-1-git-send-email-slawomirx.mrozowicz@intel.com> X-Mailer: git-send-email 1.9.1 Subject: [dpdk-dev] [PATCH v3] examples/qos_meter: fix unchecked return value X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Slawomir Mrozowicz
May 9, 2016, 8:38 a.m. UTC
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(-)
Comments
> -----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
>-----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
> -----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
>-----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
> -----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
> -----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
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