Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.

[Fischetti, Antonio]

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Monday, September 25, 2017 8:35 PM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> 
> 
> 
> On 9/25/17, 2:51 AM, "Fischetti, Antonio" <antonio.fische...@intel.com> wrote:
> 
> > -Original Message-
> > From: Darrell Ball [mailto:db...@vmware.com]
> > Sent: Friday, September 22, 2017 9:26 AM
> > To: Fischetti, Antonio <antonio.fische...@intel.com>; 
> d...@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> >
> >
> >
> > 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:
> >
> > ct_dpif_entry_uninit could potentially be called even if
> > ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
> > a pointer to a CT entry - and just checks it is not null -
> > it's safer to init to zero any instantiated ct_dpif_entry
> > variable before its usage.
> >
> > [Darrell] I took a look and did not see a particular problem.
> >Was there an issue that we are trying to address?; if so,
> this
> > may hide it ?
> 
> [Antonio]
> This change is more a matter of keeping safe habits for future
> code additions.
> In a new CT function that could be added down the line, one could
> potentially call ct_dpif_entry_uninit without checking what was
> returned by ct_dpif_dump_next.
> 
> As this is not in the hotpath, I added a memset to be extra-careful
> when initializing the local CT entry variable.
> 
> Maybe also a comment on top of the fn definition could help on this,
> something like?
> 
> +/* This function must be called when the returned
> +   value from ct_dpif_dump_next is 0. */
> void
> ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> {
> if (entry) {
> if (entry->helper.name) {
> 
> 
> [Darrell] It is usually better to wait for the new code that might need this
> and
>associate those patches as part of the same series.
>Can we do that ?

[Antonio] Yes, makes sense. I'll keep this one into my backlog.


> 
> 
> 
> >
> >
> > Signed-off-by: Antonio Fischetti <antonio.fische...@intel.com>
> > ---
> >  lib/dpctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 86d0f90..77d4e58 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char
> *argv[],
> >  return error;
> >  }
> >
> > +memset(, 0, sizeof(cte));
> >  while (!(ret = ct_dpif_dump_next(dump, ))) {
> >  struct ds s = DS_EMPTY_INITIALIZER;
> >
> > @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char
> *argv[],
> >  return error;
> >  }
> >
> > +memset(, 0, sizeof(cte));
> >  int tot_conn = 0;
> >  while (!(ret = ct_dpif_dump_next(dump, ))) {
> >  ct_dpif_entry_uninit();
> > @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> >   return 0;
> >  }
> >
> > +memset(, 0, sizeof(cte));
> >  dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
> >
> >  int tot_conn = 0;
> > --
> > 2.4.11
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> > 2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-
> >
> uZnsw=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So=tU4fSt243XI_2QHkAF4R2h0sm
> > vtTC8fDyiOXBI02_t8=
> >
> 
> 

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

Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.

[Darrell Ball]

On 9/25/17, 2:51 AM, "Fischetti, Antonio" <antonio.fische...@intel.com> wrote:

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Friday, September 22, 2017 9:26 AM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@openvswitch.org
    > Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> 
> 
> 
> 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:
> 
> ct_dpif_entry_uninit could potentially be called even if
> ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
> a pointer to a CT entry - and just checks it is not null -
> it's safer to init to zero any instantiated ct_dpif_entry
> variable before its usage.
> 
> [Darrell] I took a look and did not see a particular problem.
>Was there an issue that we are trying to address?; if so, 
this
> may hide it ?

[Antonio]
This change is more a matter of keeping safe habits for future
code additions. 
In a new CT function that could be added down the line, one could
potentially call ct_dpif_entry_uninit without checking what was
returned by ct_dpif_dump_next.

As this is not in the hotpath, I added a memset to be extra-careful 
when initializing the local CT entry variable.

Maybe also a comment on top of the fn definition could help on this,
something like?

+/* This function must be called when the returned
+   value from ct_dpif_dump_next is 0. */
void
ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
{
if (entry) {
if (entry->helper.name) {


[Darrell] It is usually better to wait for the new code that might need this and
   associate those patches as part of the same series.
   Can we do that ?



> 
> 
> Signed-off-by: Antonio Fischetti <antonio.fische...@intel.com>
> ---
>  lib/dpctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 86d0f90..77d4e58 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char 
*argv[],
>  return error;
>  }
> 
> +memset(, 0, sizeof(cte));
>  while (!(ret = ct_dpif_dump_next(dump, ))) {
>  struct ds s = DS_EMPTY_INITIALIZER;
> 
> @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char 
*argv[],
>  return error;
>  }
> 
> +memset(, 0, sizeof(cte));
>  int tot_conn = 0;
>  while (!(ret = ct_dpif_dump_next(dump, ))) {
>  ct_dpif_entry_uninit();
> @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
>   return 0;
>  }
> 
> +memset(, 0, sizeof(cte));
>  dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
> 
>  int tot_conn = 0;
> --
> 2.4.11
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-
> 
uZnsw=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So=tU4fSt243XI_2QHkAF4R2h0sm
> vtTC8fDyiOXBI02_t8=
> 



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

Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.

[Fischetti, Antonio]

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Friday, September 22, 2017 9:26 AM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> 
> 
> 
> 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:
> 
> ct_dpif_entry_uninit could potentially be called even if
> ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
> a pointer to a CT entry - and just checks it is not null -
> it's safer to init to zero any instantiated ct_dpif_entry
> variable before its usage.
> 
> [Darrell] I took a look and did not see a particular problem.
>Was there an issue that we are trying to address?; if so, this
> may hide it ?

[Antonio]
This change is more a matter of keeping safe habits for future
code additions. 
In a new CT function that could be added down the line, one could
potentially call ct_dpif_entry_uninit without checking what was
returned by ct_dpif_dump_next.

As this is not in the hotpath, I added a memset to be extra-careful 
when initializing the local CT entry variable.

Maybe also a comment on top of the fn definition could help on this,
something like?

+/* This function must be called when the returned
+   value from ct_dpif_dump_next is 0. */
void
ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
{
if (entry) {
if (entry->helper.name) {



> 
> 
> Signed-off-by: Antonio Fischetti <antonio.fische...@intel.com>
> ---
>  lib/dpctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 86d0f90..77d4e58 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
>  return error;
>  }
> 
> +memset(, 0, sizeof(cte));
>  while (!(ret = ct_dpif_dump_next(dump, ))) {
>  struct ds s = DS_EMPTY_INITIALIZER;
> 
> @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
>  return error;
>  }
> 
> +memset(, 0, sizeof(cte));
>  int tot_conn = 0;
>  while (!(ret = ct_dpif_dump_next(dump, ))) {
>  ct_dpif_entry_uninit();
> @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
>   return 0;
>  }
> 
> +memset(, 0, sizeof(cte));
>  dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
> 
>  int tot_conn = 0;
> --
> 2.4.11
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-
> uZnsw=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So=tU4fSt243XI_2QHkAF4R2h0sm
> vtTC8fDyiOXBI02_t8=
> 

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

Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.

[Darrell Ball]

On 9/13/17, 5:37 AM, "ovs-dev-boun...@openvswitch.org on behalf of 
antonio.fische...@intel.com"  wrote:

ct_dpif_entry_uninit could potentially be called even if
ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
a pointer to a CT entry - and just checks it is not null -
it's safer to init to zero any instantiated ct_dpif_entry
variable before its usage.

[Darrell] I took a look and did not see a particular problem.
   Was there an issue that we are trying to address?; if so, this 
may hide it ?


Signed-off-by: Antonio Fischetti 
---
 lib/dpctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 86d0f90..77d4e58 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
 return error;
 }
 
+memset(, 0, sizeof(cte));
 while (!(ret = ct_dpif_dump_next(dump, ))) {
 struct ds s = DS_EMPTY_INITIALIZER;
 
@@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
 return error;
 }
 
+memset(, 0, sizeof(cte));
 int tot_conn = 0;
 while (!(ret = ct_dpif_dump_next(dump, ))) {
 ct_dpif_entry_uninit();
@@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
  return 0;
 }
 
+memset(, 0, sizeof(cte));
 dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
 
 int tot_conn = 0;
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So=tU4fSt243XI_2QHkAF4R2h0smvtTC8fDyiOXBI02_t8=
 


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

Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.

[Greg Rose]

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

ct_dpif_entry_uninit could potentially be called even if
ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
a pointer to a CT entry - and just checks it is not null -
it's safer to init to zero any instantiated ct_dpif_entry
variable before its usage.

Signed-off-by: Antonio Fischetti 
---
  lib/dpctl.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 86d0f90..77d4e58 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
  return error;
  }
  
+memset(, 0, sizeof(cte));

  while (!(ret = ct_dpif_dump_next(dump, ))) {
  struct ds s = DS_EMPTY_INITIALIZER;
  
@@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],

  return error;
  }
  
+memset(, 0, sizeof(cte));

  int tot_conn = 0;
  while (!(ret = ct_dpif_dump_next(dump, ))) {
  ct_dpif_entry_uninit();
@@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
   return 0;
  }
  
+memset(, 0, sizeof(cte));

  dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
  
  int tot_conn = 0;




Not in the hotpath so OK to be extra careful here.

Reviewed-by: Greg Rose 

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

[ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.

[antonio . fischetti]

ct_dpif_entry_uninit could potentially be called even if
ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
a pointer to a CT entry - and just checks it is not null -
it's safer to init to zero any instantiated ct_dpif_entry
variable before its usage.

Signed-off-by: Antonio Fischetti 
---
 lib/dpctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 86d0f90..77d4e58 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
 return error;
 }
 
+memset(, 0, sizeof(cte));
 while (!(ret = ct_dpif_dump_next(dump, ))) {
 struct ds s = DS_EMPTY_INITIALIZER;
 
@@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
 return error;
 }
 
+memset(, 0, sizeof(cte));
 int tot_conn = 0;
 while (!(ret = ct_dpif_dump_next(dump, ))) {
 ct_dpif_entry_uninit();
@@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
  return 0;
 }
 
+memset(, 0, sizeof(cte));
 dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
 
 int tot_conn = 0;
-- 
2.4.11

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