[ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT entries.

Fischetti, Antonio antonio.fischetti at intel.com
Mon Sep 25 09:27:33 UTC 2017


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 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 ?
> 
> 
>          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, &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.
> 
>          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