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

[Fischetti, Antonio]

> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Friday, September 22, 2017 9:26 AM
> To: Fischetti, Antonio <antonio.fischetti at intel.com>; dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> 
> 
> 
> 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:
> 
>     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.fischetti at 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(&cte, 0, sizeof(cte));
>          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>              struct ds s = DS_EMPTY_INITIALIZER;
> 
>     @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
>              return error;
>          }
> 
>     +    memset(&cte, 0, sizeof(cte));
>          int tot_conn = 0;
>          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>              ct_dpif_entry_uninit(&cte);
>     @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
>               return 0;
>          }
> 
>     +    memset(&cte, 0, sizeof(cte));
>          dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
> 
>          int tot_conn = 0;
>     --
>     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=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So&s=tU4fSt243XI_2QHkAF4R2h0sm
> vtTC8fDyiOXBI02_t8&e=
>