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

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.
Related show

Commit Message

Fischetti, Antonio Sept. 13, 2017, 12:36 p.m.
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(-)

Comments

Gregory Rose Sept. 18, 2017, 6:08 p.m. | #1
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, &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;
> +    }
>   
>       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>
Darrell Ball Sept. 22, 2017, 7:42 a.m. | #2
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, &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@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=
Fischetti, Antonio Sept. 25, 2017, 9:27 a.m. | #3
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, &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@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=

>
Darrell Ball Sept. 25, 2017, 7:31 p.m. | #4
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, &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@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=

    >
Fischetti, Antonio Sept. 26, 2017, 9:09 a.m. | #5
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, &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@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=

>     >

> 

>

Patch

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, &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;
+    }
 
     dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn);
     dpctl_print(dpctl_p, "\n");