[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, &gt)) {
>     >     @@ -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