| Message ID | 1505306164-6665-1-git-send-email-antonio.fischetti@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Darrell Ball |
| Headers | show |
| Series | [ovs-dev,1/2] dpctl: manage ret value when dumping CT entries. | expand |
On 09/13/2017 05:36 AM, antonio.fischetti@intel.com wrote: > Manage error value returned by ct_dpif_dump_next. > > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> > --- > lib/dpctl.c | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 8951d6e..86d0f90 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char *argv[], > struct dpif *dpif; > char *name; > int error; > + int ret; > > if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) { > pzone = &zone; > @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[], > return error; > } > > - while (!ct_dpif_dump_next(dump, &cte)) { > + while (!(ret = ct_dpif_dump_next(dump, &cte))) { > struct ds s = DS_EMPTY_INITIALIZER; > > ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity, > @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[], > dpctl_print(dpctl_p, "%s\n", ds_cstr(&s)); > ds_destroy(&s); > } > + if (ret && ret != EOF) { > + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); > + ct_dpif_dump_done(dump); > + dpif_close(dpif); > + return ret; > + } > + > ct_dpif_dump_done(dump); > dpif_close(dpif); > return error; > @@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char *argv[], > int proto_stats[CT_STATS_MAX]; > int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM]; > int error; > + int ret; > > while (argc > 1 && lastargc != argc) { > lastargc = argc; > @@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[], > } > > int tot_conn = 0; > - while (!ct_dpif_dump_next(dump, &cte)) { > + while (!(ret = ct_dpif_dump_next(dump, &cte))) { > ct_dpif_entry_uninit(&cte); > tot_conn++; > switch (cte.tuple_orig.ip_proto) { > @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char *argv[], > break; > } > } > + if (ret && ret != EOF) { > + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); > + ct_dpif_dump_done(dump); > + dpif_close(dpif); > + return ret; > + } > > dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", tot_conn); > if (proto_stats[CT_STATS_TCP]) { > @@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[], > uint16_t *pzone = NULL; > int tot_bkts = 0; > int error; > + int ret; > > if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT, strlen(CT_BKTS_GT))) { > if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) { > @@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[], > int tot_conn = 0; > uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t)); > > - while (!ct_dpif_dump_next(dump, &cte)) { > + while (!(ret = ct_dpif_dump_next(dump, &cte))) { > ct_dpif_entry_uninit(&cte); > tot_conn++; > if (tot_bkts > 0) { > @@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[], > } > } > } > + if (ret && ret != EOF) { > + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); > + ct_dpif_dump_done(dump); > + dpif_close(dpif); > + return ret; > + } > > dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn); > dpctl_print(dpctl_p, "\n"); > This looks fine to me but I don't know of any way to test it - I guess we'd need to force a dump error. Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Few comments Antonio
Darrell
On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com> wrote:
Manage error value returned by ct_dpif_dump_next.
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
lib/dpctl.c | 28 +++++++++++++++++++++++++---
1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 8951d6e..86d0f90 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
struct dpif *dpif;
char *name;
int error;
+ int ret;
if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
pzone = &zone;
@@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
return error;
}
- while (!ct_dpif_dump_next(dump, &cte)) {
+ while (!(ret = ct_dpif_dump_next(dump, &cte))) {
struct ds s = DS_EMPTY_INITIALIZER;
ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity,
@@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
ds_destroy(&s);
}
+ if (ret && ret != EOF) {
+ dpctl_error(dpctl_p, ret, "dumping conntrack entry");
+ ct_dpif_dump_done(dump);
+ dpif_close(dpif);
+ return ret;
+ }
+
[Darrell] Maybe we can reuse ‘error’ ?
if (error && error != EOF) {
and just do
dpctl_error(dpctl_p, error, "dumping conntrack entry");
and then fall thru for cleanup ?
ct_dpif_dump_done(dump);
dpif_close(dpif);
return error;
@@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
int proto_stats[CT_STATS_MAX];
int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM];
int error;
+ int ret;
while (argc > 1 && lastargc != argc) {
lastargc = argc;
@@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
}
int tot_conn = 0;
- while (!ct_dpif_dump_next(dump, &cte)) {
+ while (!(ret = ct_dpif_dump_next(dump, &cte))) {
ct_dpif_entry_uninit(&cte);
tot_conn++;
switch (cte.tuple_orig.ip_proto) {
@@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char *argv[],
break;
}
}
+ if (ret && ret != EOF) {
+ dpctl_error(dpctl_p, ret, "dumping conntrack entry");
+ ct_dpif_dump_done(dump);
+ dpif_close(dpif);
+ return ret;
+ }
[Darrell]
Can we reuse ‘error’, just print error and fall thru. ?
It looks like it is safe to print whatever we know, which could be useful.
Otherwise, if we have an error in dump_next, we may never be able to see any useful info.
for debugging the same error or something else.
dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", tot_conn);
if (proto_stats[CT_STATS_TCP]) {
@@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
uint16_t *pzone = NULL;
int tot_bkts = 0;
int error;
+ int ret;
if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT, strlen(CT_BKTS_GT))) {
if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) {
@@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
int tot_conn = 0;
uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t));
- while (!ct_dpif_dump_next(dump, &cte)) {
+ while (!(ret = ct_dpif_dump_next(dump, &cte))) {
ct_dpif_entry_uninit(&cte);
tot_conn++;
if (tot_bkts > 0) {
@@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[],
}
}
}
+ if (ret && ret != EOF) {
+ dpctl_error(dpctl_p, ret, "dumping conntrack entry");
+ ct_dpif_dump_done(dump);
+ dpif_close(dpif);
+ return ret;
+ }
[Darrell]
Same comment here:
Can we reuse ‘error’, just print error and fall thru. ?
It looks like it is safe to print whatever we know, which could be useful.
Otherwise, if we have an error in dump_next, we may never be able to see any useful info.
for debugging the same error or something else.
dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn);
dpctl_print(dpctl_p, "\n");
--
2.4.11
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=HjzfhOU1f9U5hUM2bWAuizS921ZAAEjbIRHSM65FIAQ&s=u0r4kn1mAoWiO_rjM95TwOt5t-7hC2jiO3y_QKHHXNw&e=
Hi Darrell,
I agree with your suggestion in keeping 'error'
as the only variable to manage return values.
In this case - as I'm assuming we shouldn't return an EOF to the
caller - I should manage error as below?
if (error == EOF) {
error = 0; << EOF is not an issue, so return 0 to the caller
} else if (error) {
dpctl_error(dpctl_p, error, "dumping conntrack entry");
ct_dpif_dump_done(dump);
dpif_close(dpif);
return error;
}
> -----Original Message-----
> From: Darrell Ball [mailto:dball@vmware.com]
> Sent: Friday, September 22, 2017 8:42 AM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT
> entries.
>
> Few comments Antonio
>
> Darrell
>
> On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of
> antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of
> antonio.fischetti@intel.com> wrote:
>
> Manage error value returned by ct_dpif_dump_next.
>
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
> lib/dpctl.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 8951d6e..86d0f90 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> struct dpif *dpif;
> char *name;
> int error;
> + int ret;
>
> if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
> pzone = &zone;
> @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> return error;
> }
>
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> struct ds s = DS_EMPTY_INITIALIZER;
>
> ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity,
> @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
> ds_destroy(&s);
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
> +
>
> [Darrell] Maybe we can reuse ‘error’ ?
> if (error && error != EOF) {
> and just do
> dpctl_error(dpctl_p, error, "dumping conntrack entry");
> and then fall thru for cleanup ?
>
>
> ct_dpif_dump_done(dump);
> dpif_close(dpif);
> return error;
> @@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> int proto_stats[CT_STATS_MAX];
> int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM];
> int error;
> + int ret;
>
> while (argc > 1 && lastargc != argc) {
> lastargc = argc;
> @@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> }
>
> int tot_conn = 0;
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> ct_dpif_entry_uninit(&cte);
> tot_conn++;
> switch (cte.tuple_orig.ip_proto) {
> @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> break;
> }
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
>
> [Darrell]
> Can we reuse ‘error’, just print error and fall thru. ?
> It looks like it is safe to print whatever we know, which could be useful.
> Otherwise, if we have an error in dump_next, we may never be able to see any
> useful info.
> for debugging the same error or something else.
>
>
> dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", tot_conn);
> if (proto_stats[CT_STATS_TCP]) {
> @@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> uint16_t *pzone = NULL;
> int tot_bkts = 0;
> int error;
> + int ret;
>
> if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT,
> strlen(CT_BKTS_GT))) {
> if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) {
> @@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> int tot_conn = 0;
> uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t));
>
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> ct_dpif_entry_uninit(&cte);
> tot_conn++;
> if (tot_bkts > 0) {
> @@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[],
> }
> }
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
>
> [Darrell]
> Same comment here:
> Can we reuse ‘error’, just print error and fall thru. ?
> It looks like it is safe to print whatever we know, which could be useful.
> Otherwise, if we have an error in dump_next, we may never be able to see any
> useful info.
> for debugging the same error or something else.
>
> dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn);
> dpctl_print(dpctl_p, "\n");
> --
> 2.4.11
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
> uZnsw&m=HjzfhOU1f9U5hUM2bWAuizS921ZAAEjbIRHSM65FIAQ&s=u0r4kn1mAoWiO_rjM95TwOt5t
> -7hC2jiO3y_QKHHXNw&e=
>
On 9/25/17, 2:27 AM, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote:
Hi Darrell,
I agree with your suggestion in keeping 'error'
as the only variable to manage return values.
In this case - as I'm assuming we shouldn't return an EOF to the
caller - I should manage error as below?
if (error == EOF) {
error = 0; << EOF is not an issue, so return 0 to the caller
} else if (error) {
dpctl_error(dpctl_p, error, "dumping conntrack entry");
ct_dpif_dump_done(dump);
dpif_close(dpif);
return error;
}
[Darrell] For sure - EOF should not be returned to user since it is not an error.
The other point I wanted to make is:
I think you can trivially fall thru. for dpctl_dump_conntrack()
After doing just dpctl_error(dpctl_p, error, "dumping conntrack entry");
(comments inline).
And in the other 2 cases, I think you can still try to print out whatever is known
after breaking out of the while loop and use the existing cleanup code.
You would print error in the cases as well
What do you think ?
> -----Original Message-----
> From: Darrell Ball [mailto:dball@vmware.com]
> Sent: Friday, September 22, 2017 8:42 AM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT
> entries.
>
> Few comments Antonio
>
> Darrell
>
> On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of
> antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of
> antonio.fischetti@intel.com> wrote:
>
> Manage error value returned by ct_dpif_dump_next.
>
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
> lib/dpctl.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 8951d6e..86d0f90 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> struct dpif *dpif;
> char *name;
> int error;
> + int ret;
>
> if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
> pzone = &zone;
> @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> return error;
> }
>
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> struct ds s = DS_EMPTY_INITIALIZER;
>
> ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity,
> @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
> ds_destroy(&s);
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
> +
>
> [Darrell] Maybe we can reuse ‘error’ ?
> if (error && error != EOF) {
> and just do
> dpctl_error(dpctl_p, error, "dumping conntrack entry");
> and then fall thru for cleanup ?
>
>
> ct_dpif_dump_done(dump);
> dpif_close(dpif);
> return error;
> @@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> int proto_stats[CT_STATS_MAX];
> int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM];
> int error;
> + int ret;
>
> while (argc > 1 && lastargc != argc) {
> lastargc = argc;
> @@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> }
>
> int tot_conn = 0;
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> ct_dpif_entry_uninit(&cte);
> tot_conn++;
> switch (cte.tuple_orig.ip_proto) {
> @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> break;
> }
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
>
> [Darrell]
> Can we reuse ‘error’, just print error and fall thru. ?
> It looks like it is safe to print whatever we know, which could be useful.
> Otherwise, if we have an error in dump_next, we may never be able to see any
> useful info.
> for debugging the same error or something else.
>
>
> dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", tot_conn);
> if (proto_stats[CT_STATS_TCP]) {
> @@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> uint16_t *pzone = NULL;
> int tot_bkts = 0;
> int error;
> + int ret;
>
> if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT,
> strlen(CT_BKTS_GT))) {
> if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) {
> @@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> int tot_conn = 0;
> uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t));
>
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> ct_dpif_entry_uninit(&cte);
> tot_conn++;
> if (tot_bkts > 0) {
> @@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[],
> }
> }
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
>
> [Darrell]
> Same comment here:
> Can we reuse ‘error’, just print error and fall thru. ?
> It looks like it is safe to print whatever we know, which could be useful.
> Otherwise, if we have an error in dump_next, we may never be able to see any
> useful info.
> for debugging the same error or something else.
>
> dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn);
> dpctl_print(dpctl_p, "\n");
> --
> 2.4.11
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
> uZnsw&m=HjzfhOU1f9U5hUM2bWAuizS921ZAAEjbIRHSM65FIAQ&s=u0r4kn1mAoWiO_rjM95TwOt5t
> -7hC2jiO3y_QKHHXNw&e=
>
Thanks Darrell, comments inline. I'll post a v2 based on your suggestions. -Antonio > -----Original Message----- > From: Darrell Ball [mailto:dball@vmware.com] > Sent: Monday, September 25, 2017 8:31 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT > entries. > > > > On 9/25/17, 2:27 AM, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote: > > Hi Darrell, > I agree with your suggestion in keeping 'error' > as the only variable to manage return values. > > In this case - as I'm assuming we shouldn't return an EOF to the > caller - I should manage error as below? > > if (error == EOF) { > error = 0; << EOF is not an issue, so return 0 to the caller > } else if (error) { > dpctl_error(dpctl_p, error, "dumping conntrack entry"); > ct_dpif_dump_done(dump); > dpif_close(dpif); > return error; > } > > > [Darrell] For sure - EOF should not be returned to user since it is not an > error. > The other point I wanted to make is: > I think you can trivially fall thru. for dpctl_dump_conntrack() > After doing just dpctl_error(dpctl_p, error, "dumping > conntrack entry"); > (comments inline). > And in the other 2 cases, I think you can still try to print out > whatever is known > after breaking out of the while loop and use the existing > cleanup code. > You would print error in the cases as well > What do you think ? [Antonio] Good idea, thanks. So for example the CT protocol stats or anything else could be displayed anyway. > > > > > > -----Original Message----- > > From: Darrell Ball [mailto:dball@vmware.com] > > Sent: Friday, September 22, 2017 8:42 AM > > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping > CT > > entries. > > > > Few comments Antonio > > > > Darrell > > > > On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of > > antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf > of > > antonio.fischetti@intel.com> wrote: > > > > Manage error value returned by ct_dpif_dump_next. > > > > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> > > --- > > lib/dpctl.c | 28 +++++++++++++++++++++++++--- > > 1 file changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 8951d6e..86d0f90 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char > *argv[], > > struct dpif *dpif; > > char *name; > > int error; > > + int ret; > > > > if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) > { > > pzone = &zone; > > @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char > *argv[], > > return error; > > } > > > > - while (!ct_dpif_dump_next(dump, &cte)) { > > + while (!(ret = ct_dpif_dump_next(dump, &cte))) { > > struct ds s = DS_EMPTY_INITIALIZER; > > > > ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity, > > @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char > *argv[], > > dpctl_print(dpctl_p, "%s\n", ds_cstr(&s)); > > ds_destroy(&s); > > } > > + if (ret && ret != EOF) { > > + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); > > + ct_dpif_dump_done(dump); > > + dpif_close(dpif); > > + return ret; > > + } > > + > > > > [Darrell] Maybe we can reuse ‘error’ ? > > if (error && error != EOF) { > > and just do > > dpctl_error(dpctl_p, error, "dumping conntrack entry"); > > and then fall thru for cleanup ? [Antonio] Yes, will do that. > > > > > > ct_dpif_dump_done(dump); > > dpif_close(dpif); > > return error; > > @@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char > *argv[], > > int proto_stats[CT_STATS_MAX]; > > int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM]; > > int error; > > + int ret; > > > > while (argc > 1 && lastargc != argc) { > > lastargc = argc; > > @@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char > *argv[], > > } > > > > int tot_conn = 0; > > - while (!ct_dpif_dump_next(dump, &cte)) { > > + while (!(ret = ct_dpif_dump_next(dump, &cte))) { > > ct_dpif_entry_uninit(&cte); > > tot_conn++; > > switch (cte.tuple_orig.ip_proto) { > > @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char > *argv[], > > break; > > } > > } > > + if (ret && ret != EOF) { > > + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); > > + ct_dpif_dump_done(dump); > > + dpif_close(dpif); > > + return ret; > > + } > > > > [Darrell] > > Can we reuse ‘error’, just print error and fall thru. ? > > It looks like it is safe to print whatever we know, which could be > useful. > > Otherwise, if we have an error in dump_next, we may never be able to see > any > > useful info. > > for debugging the same error or something else. [Antonio] Good idea, will do that. > > > > > > dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", > tot_conn); > > if (proto_stats[CT_STATS_TCP]) { > > @@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[], > > uint16_t *pzone = NULL; > > int tot_bkts = 0; > > int error; > > + int ret; > > > > if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT, > > strlen(CT_BKTS_GT))) { > > if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) { > > @@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[], > > int tot_conn = 0; > > uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t)); > > > > - while (!ct_dpif_dump_next(dump, &cte)) { > > + while (!(ret = ct_dpif_dump_next(dump, &cte))) { > > ct_dpif_entry_uninit(&cte); > > tot_conn++; > > if (tot_bkts > 0) { > > @@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[], > > } > > } > > } > > + if (ret && ret != EOF) { > > + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); > > + ct_dpif_dump_done(dump); > > + dpif_close(dpif); > > + return ret; > > + } > > > > [Darrell] > > Same comment here: > > Can we reuse ‘error’, just print error and fall thru. ? > > It looks like it is safe to print whatever we know, which could be > useful. > > Otherwise, if we have an error in dump_next, we may never be able to see > any > > useful info. > > for debugging the same error or something else. [Antonio] Yes, will do that. > > > > dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn); > > dpctl_print(dpctl_p, "\n"); > > -- > > 2.4.11 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__mail.openvswitch.org_mailman_listinfo_ovs- > > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih- > > > uZnsw&m=HjzfhOU1f9U5hUM2bWAuizS921ZAAEjbIRHSM65FIAQ&s=u0r4kn1mAoWiO_rjM95TwOt5t > > -7hC2jiO3y_QKHHXNw&e= > > > >
diff --git a/lib/dpctl.c b/lib/dpctl.c index 8951d6e..86d0f90 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char *argv[], struct dpif *dpif; char *name; int error; + int ret; if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) { pzone = &zone; @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[], return error; } - while (!ct_dpif_dump_next(dump, &cte)) { + while (!(ret = ct_dpif_dump_next(dump, &cte))) { struct ds s = DS_EMPTY_INITIALIZER; ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity, @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[], dpctl_print(dpctl_p, "%s\n", ds_cstr(&s)); ds_destroy(&s); } + if (ret && ret != EOF) { + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); + ct_dpif_dump_done(dump); + dpif_close(dpif); + return ret; + } + ct_dpif_dump_done(dump); dpif_close(dpif); return error; @@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char *argv[], int proto_stats[CT_STATS_MAX]; int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM]; int error; + int ret; while (argc > 1 && lastargc != argc) { lastargc = argc; @@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[], } int tot_conn = 0; - while (!ct_dpif_dump_next(dump, &cte)) { + while (!(ret = ct_dpif_dump_next(dump, &cte))) { ct_dpif_entry_uninit(&cte); tot_conn++; switch (cte.tuple_orig.ip_proto) { @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char *argv[], break; } } + if (ret && ret != EOF) { + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); + ct_dpif_dump_done(dump); + dpif_close(dpif); + return ret; + } dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", tot_conn); if (proto_stats[CT_STATS_TCP]) { @@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[], uint16_t *pzone = NULL; int tot_bkts = 0; int error; + int ret; if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT, strlen(CT_BKTS_GT))) { if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) { @@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[], int tot_conn = 0; uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t)); - while (!ct_dpif_dump_next(dump, &cte)) { + while (!(ret = ct_dpif_dump_next(dump, &cte))) { ct_dpif_entry_uninit(&cte); tot_conn++; if (tot_bkts > 0) { @@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[], } } } + if (ret && ret != EOF) { + dpctl_error(dpctl_p, ret, "dumping conntrack entry"); + ct_dpif_dump_done(dump); + dpif_close(dpif); + return ret; + } dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn); dpctl_print(dpctl_p, "\n");
Manage error value returned by ct_dpif_dump_next. Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> --- lib/dpctl.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)