On 9/25/17, 2:51 AM, "Fischetti, Antonio" <[email protected]> wrote:
> -----Original Message-----
> From: Darrell Ball [mailto:[email protected]]
> Sent: Friday, September 22, 2017 9:26 AM
> To: Fischetti, Antonio <[email protected]>; [email protected]
> Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
>
>
>
> On 9/13/17, 5:37 AM, "[email protected] on behalf of
> [email protected]" <[email protected] on behalf of
> [email protected]> 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 <[email protected]>
> ---
> 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
> [email protected]
> 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=
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
