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

[Fischetti, Antonio]

Thanks Darrell, comments inline. I'll post a v2 based on your suggestions.

-Antonio

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Monday, September 25, 2017 8:31 PM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@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.fische...@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:db...@vmware.com]
> > Sent: Friday, September 22, 2017 8:42 AM
> > To: Fischetti, Antonio <antonio.fische...@intel.com>; 
> d...@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-boun...@openvswitch.org on behalf of
> > antonio.fische...@intel.com" <ovs-dev-boun...@openvswitch.org on behalf
> of
> > antonio.fische...@intel.com> wrote:
> >
> > Manage error value returned by ct_dpif_dump_next.
> >
> > Signed-off-by: Antonio Fischetti <antonio.fische...@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, 
> ))
> {
> >  pzone = 
> > @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char
> *argv[],
> >  return error;
> >  }
> >
> > -while (!ct_dpif_dump_next(dump, )) {
> > +while (!(ret = ct_dpif_dump_next(dump, ))) {
> >  struct ds s = DS_EMPTY_INITIALIZER;
> >
> >  ct_dpif_format_entry(, , dpctl_p->verbosity,
> > @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char
> *argv[],
> >  dpctl_print(dpctl_p, "%s\n", ds_cstr());
> >  ds_destroy();
> >  }
> > +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 

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

[Darrell Ball]

On 9/25/17, 2:27 AM, "Fischetti, Antonio" <antonio.fische...@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:db...@vmware.com]
> Sent: Friday, September 22, 2017 8:42 AM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@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-boun...@openvswitch.org on behalf of
> antonio.fische...@intel.com" <ovs-dev-boun...@openvswitch.org on behalf of
> antonio.fische...@intel.com> wrote:
> 
> Manage error value returned by ct_dpif_dump_next.
> 
> Signed-off-by: Antonio Fischetti <antonio.fische...@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, )) 
{
>  pzone = 
> @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char 
*argv[],
>  return error;
>  }
> 
> -while (!ct_dpif_dump_next(dump, )) {
> +while (!(ret = ct_dpif_dump_next(dump, ))) {
>  struct ds s = DS_EMPTY_INITIALIZER;
> 
>  ct_dpif_format_entry(, , dpctl_p->verbosity,
> @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char 
*argv[],
>  dpctl_print(dpctl_p, "%s\n", ds_cstr());
>  ds_destroy();
>  }
> +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, )) {
> +while (!(ret = ct_dpif_dump_next(dump, ))) {
>  ct_dpif_entry_uninit();
>  tot_conn++;
>  switch (cte.tuple_orig.ip_proto) {
> @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char 
*

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

[Fischetti, Antonio]

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:db...@vmware.com]
> Sent: Friday, September 22, 2017 8:42 AM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@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-boun...@openvswitch.org on behalf of
> antonio.fische...@intel.com" <ovs-dev-boun...@openvswitch.org on behalf of
> antonio.fische...@intel.com> wrote:
> 
> Manage error value returned by ct_dpif_dump_next.
> 
> Signed-off-by: Antonio Fischetti <antonio.fische...@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, )) {
>  pzone = 
> @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
>  return error;
>  }
> 
> -while (!ct_dpif_dump_next(dump, )) {
> +while (!(ret = ct_dpif_dump_next(dump, ))) {
>  struct ds s = DS_EMPTY_INITIALIZER;
> 
>  ct_dpif_format_entry(, , dpctl_p->verbosity,
> @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
>  dpctl_print(dpctl_p, "%s\n", ds_cstr());
>  ds_destroy();
>  }
> +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, )) {
> +while (!(ret = ct_dpif_dump_next(dump, ))) {
>  ct_dpif_entry_uninit();
>  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:\nTotal: %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 (o

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

[Greg Rose]

On 09/13/2017 05:36 AM, antonio.fische...@intel.com wrote:

Manage error value returned by ct_dpif_dump_next.

Signed-off-by: Antonio Fischetti 
---
  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, )) {

  pzone = 
@@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
  return error;
  }
  
-while (!ct_dpif_dump_next(dump, )) {

+while (!(ret = ct_dpif_dump_next(dump, ))) {
  struct ds s = DS_EMPTY_INITIALIZER;
  
  ct_dpif_format_entry(, , dpctl_p->verbosity,

@@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
  dpctl_print(dpctl_p, "%s\n", ds_cstr());
  ds_destroy();
  }
+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, )) {
+while (!(ret = ct_dpif_dump_next(dump, ))) {
  ct_dpif_entry_uninit();
  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:\nTotal: %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, )) {

+while (!(ret = ct_dpif_dump_next(dump, ))) {
  ct_dpif_entry_uninit();
  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 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[antonio . fischetti]

Manage error value returned by ct_dpif_dump_next.

Signed-off-by: Antonio Fischetti 
---
 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, )) {
 pzone = 
@@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
 return error;
 }
 
-while (!ct_dpif_dump_next(dump, )) {
+while (!(ret = ct_dpif_dump_next(dump, ))) {
 struct ds s = DS_EMPTY_INITIALIZER;
 
 ct_dpif_format_entry(, , dpctl_p->verbosity,
@@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
 dpctl_print(dpctl_p, "%s\n", ds_cstr());
 ds_destroy();
 }
+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, )) {
+while (!(ret = ct_dpif_dump_next(dump, ))) {
 ct_dpif_entry_uninit();
 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:\nTotal: %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, )) {
+while (!(ret = ct_dpif_dump_next(dump, ))) {
 ct_dpif_entry_uninit();
 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");
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev