On Fri, Jul 26, 2019 at 10:15 AM William Tu <u9012...@gmail.com> wrote: > > +static void > > +dpif_netlink_format_tp_name(uint32_t id, uint16_t l3num, uint8_t l4num, > > + struct ds *tp_name) > > +{ > > + ds_clear(tp_name); > > + ds_put_format(tp_name, "%s%"PRIu32"_", NL_TP_NAME_PREFIX, id); > > + ct_dpif_format_ipproto(tp_name, l4num); > > + > > + if (l3num == AF_INET) { > > + ds_put_cstr(tp_name, "4"); > > + } else if (l3num == AF_INET6 && l4num != IPPROTO_ICMPV6) { > > Why excluding IPPROTO_ICMPV6 above?
Thanks for review. It is because ct_dpif_format_ipproto returns "icmpv6" for IPPROTO_ICMPV6 and "icmp" for "IPPROTO_ICMP", and I found it to be confusing to have ovs_tp_<tp_id>_icmpv66 as the timeout policy name. > > +#define CT_DPIF_TO_NL_TP_MAPPING(PROTO1, PROTO2, ATTR1, ATTR2) \ > > +if (nl_tp->present & (1 << CTA_TIMEOUT_##PROTO2##_##ATTR2)) { \ > > + tp->present |= 1 << CT_DPIF_TP_ATTR_##PROTO1##_##ATTR1; \ > > + tp->attrs[CT_DPIF_TP_ATTR_##PROTO1##_##ATTR1] = \ > > + nl_tp->attrs[CTA_TIMEOUT_##PROTO2##_##ATTR2]; \ > > + } > > + > > +static void > > +dpif_netlink_set_ct_dpif_tp_tcp_attrs(const struct nl_ct_timeout_policy > > *nl_tp, > > + struct ct_dpif_timeout_policy *tp) > > +{ > > + CT_DPIF_TO_NL_TP_TCP_MAPPINGS > > Is this better to renamed as CT_DPIF_FROM_NL_TP_TCP_MAPPINGS? > > You're using the same macro name, one for > setting the nl_tp->attrs from tp->attrs, the other for > setting the tp->attrs from nl_tp_attrs Thanks for the suggestion. As our offline discussion, it is confusing to have "_TO_" in the marco name, I will get rid of it. > > +static int > > +dpif_netlink_ct_add_timeout_policy(struct dpif *dpif OVS_UNUSED, > > + bool is_default, > > + const struct ct_dpif_timeout_policy *tp) > > +{ > > +#ifdef _WIN32 > > + return EOPNOTSUPP; > > +#else > > + struct nl_ct_timeout_policy nl_tp; > > + struct ds ds = DS_EMPTY_INITIALIZER; > > + int i, err; > > + > > + for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) { > > + dpif_netlink_format_tp_name(tp->id, tp_protos[i].l3num, > > + tp_protos[i].l4num, &ds); > > + ovs_strlcpy(nl_tp.name, ds_cstr(&ds), sizeof nl_tp.name); > > + nl_tp.l3num = tp_protos[i].l3num; > > + nl_tp.l4num = tp_protos[i].l4num; > > + dpif_netlink_get_nl_tp_attrs(tp, tp_protos[i].l4num, &nl_tp); > > + if (!is_default) { > > + err = nl_ct_set_timeout_policy(&nl_tp); > > + } else if (tp_protos[i].l3num == AF_INET) { > > + /* The default timeout policy is shared between AF_INET and > > + * AF_INET6 in the kernel. So configure AF_INET is sufficient. > > */ > > + err = nl_ct_set_default_timeout_policy(&nl_tp); > > + } > > + if (err) { > > + VLOG_INFO("failed to set timeout policy %s (%s)", nl_tp.name, > > + ovs_strerror(err)); > ds_destroy(&ds); Thanks, I will destroy the dynamic string properly in all the following cases in v2. > > +static int > > +dpif_netlink_ct_get_timeout_policy(struct dpif *dpif OVS_UNUSED, > > + bool is_default, uint32_t tp_id, > > + struct ct_dpif_timeout_policy *tp) > > +{ > > +#ifdef _WIN32 > > + return EOPNOTSUPP; > > +#else > if _WIN32 is alway return EOPNOTSUPP, > is it better if we aggregate all 6 functions and have a larger > #ifdef _WIN32 > // all six functions return EOPNOTSUPP > #else > // actual implementations > #endif Sure, I will make proper change to make the code looks clearly in the next version. > > + struct nl_ct_timeout_policy nl_tp; > > + struct ds nl_tp_name = DS_EMPTY_INITIALIZER; > > + int i, err; > > + > > + tp->id = tp_id; > > + tp->present = 0; > > + for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) { > > + if (!is_default) { > > + dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num, > > + tp_protos[i].l4num, &nl_tp_name); > > + err = nl_ct_get_timeout_policy(ds_cstr(&nl_tp_name), &nl_tp); > > + } else if (tp_protos[i].l3num == AF_INET) { > > + /* The default timeout is shared between AF_INET and AF_INET6 > > + * in the kernel. So get from AF_INET is sufficient. */ > Then why checking 'tp_protos[i].l3num == AF_INET'? > What happens when tp_protos[i].l3num == AF_INET6? then 'err' becomes > uninitialized. This function is called from ct-dpif to query the timeout policy stored in the kernel. It will loop through all L3/L4 pairs (ipv4 tcp/udp/icmp and ipv6 tcp/udp/icmpv6). The main purpose for this check is to skip AF_INET6 cases for default timeout since it does not distingush the ipv4 and ipv6 cases in the kernel. > > +static int > > +dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED, > > + uint32_t tp_id) > > +{ > > +#ifdef _WIN32 > > + return EOPNOTSUPP; > > +#else > > + struct ds nl_tp_name = DS_EMPTY_INITIALIZER; > > + int i, err; > > + > > + if (!tp_id) { > > + return EINVAL; > > + } > > + > > + for (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) { > > + VLOG_INFO("failed to delete timeout policy %s (%s)", > > + ds_cstr(&nl_tp_name), ovs_strerror(err)); > Use VLOG_WARN? or VLOG_WARN_RL? I will change that to VLOG_WARN_RL in v2. > > +static int > > +dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED, > > + void *state, > > + struct ct_dpif_timeout_policy > > **tp) > > +{ > > +#ifdef _WIN32 > > + return EOPNOTSUPP; > > +#else > > + struct dpif_netlink_ct_timeout_policy_dump_state *dump_state = state; > > + struct dpif_netlink_tp_dump_node *tp_dump_node; > > + struct nl_ct_timeout_policy nl_tp; > > + uint32_t tp_id; > > + int err; > > + > > + do { > > + err = nl_ct_timeout_policy_dump_next(dump_state->nl_dump_state, > > + &nl_tp); > > + if (err) { > > + break; > > + } > > + > > + if (!ovs_scan(nl_tp.name, NL_TP_NAME_PREFIX"%"PRIu32, &tp_id)) { > > + continue; > > + } > > + > > + tp_dump_node = get_dpif_netlink_tp_dump_node_by_tp_id( > > + tp_id, &dump_state->tp_dump_map); > > + if (!tp_dump_node) { > > + tp_dump_node = xzalloc(sizeof *tp_dump_node); > > + tp_dump_node->tp = xzalloc(sizeof *tp_dump_node->tp); > > + tp_dump_node->tp->id = tp_id; > > + hmap_insert(&dump_state->tp_dump_map, &tp_dump_node->hmap_node, > > + hash_int(tp_id, 0)); > > + } > > + > > + update_dpif_netlink_tp_dump_node(&nl_tp, tp_dump_node); > > + if (tp_dump_node->present == DPIF_NL_ALL_TP) { > > + hmap_remove(&dump_state->tp_dump_map, > > &tp_dump_node->hmap_node); > > + *tp = tp_dump_node->tp; > > + free(tp_dump_node); > Do we have to remove and free tp_dump_node here? > Isn't it done at dpif_netlink_ct_timeout_policy_dump_done()? This is the case where we gather all of the 6 sub timeout policies and return that to ct-dpif layer. Once a full profile is gathered, we will report that to ct-dpif layer and reomve the tp_dump_node. What we free in dpif_netlink_ct_timeout_policy_dump_done() is the incomplete timeout policies. Thanks, -Yi-Hung _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev