[ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT entries.
Fischetti, Antonio
antonio.fischetti at intel.com
Tue Sep 26 09:09:40 UTC 2017
Thanks Darrell, comments inline. I'll post a v2 based on your suggestions.
-Antonio
> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Monday, September 25, 2017 8:31 PM
> To: Fischetti, Antonio <antonio.fischetti at intel.com>; dev at 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 at 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 at vmware.com]
> > Sent: Friday, September 22, 2017 8:42 AM
> > To: Fischetti, Antonio <antonio.fischetti at intel.com>; dev at 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 at openvswitch.org on behalf of
> > antonio.fischetti at intel.com" <ovs-dev-bounces at openvswitch.org on behalf
> of
> > antonio.fischetti at intel.com> wrote:
> >
> > Manage error value returned by ct_dpif_dump_next.
> >
> > Signed-off-by: Antonio Fischetti <antonio.fischetti at 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 at 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=
> >
>
>
More information about the dev
mailing list