[ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

Yi-Hung Wei yihung.wei at gmail.com
Tue Aug 20 19:30:17 UTC 2019


On Tue, Aug 20, 2019 at 12:46 AM Darrell Ball <dlu998 at gmail.com> wrote:
> After fixing a bug in my proposed incremental and adding tracking of an already removed sub timeout policy:
> Pls double check.

Thanks for the proposed incremental.

I checked all the other logging places in dpif-netlink, we usually do
not log the successfully cases in the INFO level.  As the discussion
in the e-mail thread, I think the successful cases does not provide
much useful information, so I made some minor changes based on the
proposed incremental.  I will fold in the following diff.

Thanks,

-Yi-Hung

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 1d4ee60bd199..85827cd65503 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif OVS_UNUSED,
                           struct ct_dpif_dump_state *dump_)
 {
     struct dpif_netlink_ct_dump_state *dump;
-    int err;

     INIT_CONTAINER(dump, dump_, up);

-    err = nl_ct_dump_done(dump->nl_ct_dump);
+    int err = nl_ct_dump_done(dump->nl_ct_dump);
     free(dump);
     return err;
 }
@@ -3318,32 +3317,32 @@ out:
     return err;
 }

-/* Returns 0 if all the sub timeout policies are deleted or
- * not exist in the kernel. */
+/* Returns 0 if all the sub timeout policies are deleted or not exist in the
+ * kernel.  Returns 1 if any sub timeout policy deletion failed. */
 static int
 dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED,
                                    uint32_t tp_id)
 {
     struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
-    int err = 0;
+    int ret = 0;

     for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
         dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
                                     tp_protos[i].l4num, &nl_tp_name);
-        err = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
+        int err = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
         if (err == ENOENT) {
             err = 0;
         }
         if (err) {
-            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy %s (%s)",
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
+            VLOG_INFO_RL(&rl, "failed to delete timeout policy %s (%s)",
                          ds_cstr(&nl_tp_name), ovs_strerror(err));
-            goto out;
+            ret = 1;
         }
     }

-out:
     ds_destroy(&nl_tp_name);
-    return err;
+    return ret;
 }

 struct dpif_netlink_ct_timeout_policy_dump_state {
@@ -3392,10 +3391,9 @@
dpif_netlink_ct_timeout_policy_dump_start(struct dpif *dpif
OVS_UNUSED,
                                           void **statep)
 {
     struct dpif_netlink_ct_timeout_policy_dump_state *dump_state;
-    int err;

     *statep = dump_state = xzalloc(sizeof *dump_state);
-    err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
+    int err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
     if (err) {
         free(dump_state);
         return err;

<------------------------- end of diff -------------------------------------->

>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 1d4ee60..cba4432 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif OVS_UNUSED,
>                            struct ct_dpif_dump_state *dump_)
>  {
>      struct dpif_netlink_ct_dump_state *dump;
> -    int err;
>
>      INIT_CONTAINER(dump, dump_, up);
>
> -    err = nl_ct_dump_done(dump->nl_ct_dump);
> +    int err = nl_ct_dump_done(dump->nl_ct_dump);
>      free(dump);
>      return err;
>  }
> @@ -3319,7 +3318,8 @@ out:
>  }
>
>  /* Returns 0 if all the sub timeout policies are deleted or
> - * not exist in the kernel. */
> + * not exist in the kernel; returns 1 if any sub timeout policy deletion
> + * failed. */
>  static int
>  dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED,
>                                     uint32_t tp_id)
> @@ -3330,18 +3330,19 @@ dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED,
>      for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
>          dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
>                                      tp_protos[i].l4num, &nl_tp_name);
> -        err = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
> -        if (err == ENOENT) {
> -            err = 0;
> -        }
> -        if (err) {
> -            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy %s (%s)",
> -                         ds_cstr(&nl_tp_name), ovs_strerror(err));
> -            goto out;
> +        int err2 = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
> +        VLOG_INFO_RL(&rl, err2 == ENOENT
> +                     ? "timeout policy already removed %s (%s)"
> +                     : !err2 ? "deleted timeout policy %s (%s)"
> +                             : "failed to delete timeout policy %s (%s)",
> +                     ds_cstr(&nl_tp_name), ovs_strerror(err2));
> +        if (err2 == ENOENT) {
> +            err2 = 0;
>          }
> +        err = err || err2;
>      }
>
> -out:
>      ds_destroy(&nl_tp_name);
>      return err;
>  }
> @@ -3392,10 +3393,9 @@ dpif_netlink_ct_timeout_policy_dump_start(struct dpif *dpif OVS_UNUSED,
>                                            void **statep)
>  {
>      struct dpif_netlink_ct_timeout_policy_dump_state *dump_state;
> -    int err;
>
>      *statep = dump_state = xzalloc(sizeof *dump_state);
> -    err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
> +    int err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
>      if (err) {
>          free(dump_state);
>          return err;
> (END)


More information about the dev mailing list