Thanks Darrell, comments inline. I'll post a v2 based on your suggestions. -Antonio
> -----Original Message----- > From: Darrell Ball [mailto:[email protected]] > Sent: Monday, September 25, 2017 8:31 PM > To: Fischetti, Antonio <[email protected]>; [email protected] > Subject: Re: [ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT > entries. > > > > On 9/25/17, 2:27 AM, "Fischetti, Antonio" <[email protected]> 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:[email protected]] > > Sent: Friday, September 22, 2017 8:42 AM > > To: Fischetti, Antonio <[email protected]>; > [email protected] > > 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, "[email protected] on behalf of > > [email protected]" <[email protected] on behalf > of > > [email protected]> wrote: > > > > Manage error value returned by ct_dpif_dump_next. > > > > Signed-off-by: Antonio Fischetti <[email protected]> > > --- > > 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 > > [email protected] > > 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= > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
