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

Yi-Hung Wei yihung.wei at gmail.com
Wed Aug 14 23:41:28 UTC 2019


On Wed, Aug 14, 2019 at 2:35 PM Darrell Ball <dlu998 at gmail.com> wrote:
> On Mon, Aug 12, 2019 at 5:54 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> +static int
>> +dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
>> +                                   const struct ct_dpif_timeout_policy *tp)
>> +{
>> +    struct nl_ct_timeout_policy nl_tp;
>> +    struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
>> +    int i, err = 0;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
>
>
> for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
>
> there are several other cases as well
> I not going to mention all

Thanks for the review. I make all the similar code changes in
ct-dpif.c, dpif-netlink.c and netlink-conntrack.c.


>> +static int
>> +dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
>> +                                         void *state,
>> +                                         struct ct_dpif_timeout_policy *tp)
>
> I think it would be super helpful to add some comments to this function.
>

Sure, I added some comments to dpif_netlink_ct_timeout_policy_dump_next() in v4.

>> +        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->l3_l4_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->tp);
>> +            free(tp_dump_node);
>
> common block; write once

Sure, I move the common block to a new function in v4.

static void
get_and_cleanup_tp_dump_node(struct hmap *hamp,
                             struct dpif_netlink_tp_dump_node *tp_dump_node,
                             struct ct_dpif_timeout_policy *tp)
{
    hmap_remove(hmap, &tp_dump_node->hmap_node);
    *tp = *tp_dump_node->tp;
    free(tp_dump_node->tp);
    free(tp_dump_node);
}

Thanks,

-Yi-Hung


More information about the dev mailing list