On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu...@gmail.com> wrote:
>
> Thanks for the patch
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review
> easier.
Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in v2.
>> +struct ct_dpif_timeout_policy {
>> + uint32_t id; /* id that uniquely identify a timeout policy.
>> */
>> + uint32_t present; /* If a timeout attribute is present set the
>> + * corresponding bit. */
>>
>> + uint32_t attrs[CT_DPIF_TP_ATTR_MAX]; /* An array that specifies
>> + * timeout attribute values
>> */
>
>
> I think you can make attrs of type 'int32_t' and use '-1' timeout for 'not
> present' and then
> remove the 'present' field
The timeout value is uint32_t in the kernel, so I will keep it as
uint32_t. I find the present flag to be handy when doing conversion
from ct-dpif layer to dpif-netlink layer, and as you mentioned in the
following e-mail, I will keep it as is.
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -3434,6 +3434,12 @@ const struct dpif_class dpif_netlink_class = {
>> dpif_netlink_ct_set_limits,
>> dpif_netlink_ct_get_limits,
>> dpif_netlink_ct_del_limits,
>> + NULL, /* ct_set_timeout_policy */
>> + NULL, /* ct_get_timeout_policy */
>> + NULL, /* ct_del_timeout_policy */
>> + NULL, /* ct_timeout_policy_dump_start */
>> + NULL, /* ct_timeout_policy_dump_next */
>> + NULL, /* ct_timeout_policy_dump_done */
>
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review
> easier.
>
>
>>
>> NULL, /* ipf_set_enabled */
>> NULL, /* ipf_set_min_frag */
>> NULL, /* ipf_set_max_nfrags */
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index 12898b9e3c6d..3460ef8aa98d 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -80,6 +80,7 @@ dpif_flow_dump_thread_init(struct dpif_flow_dump_thread
>> *thread,
>> struct ct_dpif_dump_state;
>> struct ct_dpif_entry;
>> struct ct_dpif_tuple;
>> +struct ct_dpif_timeout_policy;
>>
>> /* 'dpif_ipf_proto_status' and 'dpif_ipf_status' are presently in
>> * sync with 'ipf_proto_status' and 'ipf_status', but more
>> @@ -498,6 +499,48 @@ struct dpif_class {
>> * list of 'struct ct_dpif_zone_limit' entries. */
>> int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits);
>>
>> + /* Connection tracking timeout policy */
>> +
>> + /* A connection tracking timeout policy contains a list of timeout
>> + * attributes that specifies timeout values on various connection
>> states.
>> + * In a datapath, the timeout policy is identified by a 4 bytes unsigned
>> + * integer, and the unsupported timeout attributes are ignored.
>> + * When a connection is committed it can be associated with a timeout
>> + * policy, or it defaults to the default timeout policy. */
>> +
>> + /* Add timeout policy '*tp' into the datapath. If 'is_default' is true
>
>
> "is_default" - can you explain this one ?
This flag is used to configure the default timeout policy in the
datapath. If 'is_default' is true, it will set the provided timeout
policy to be the default timeout policy. The default timeout policy is
documented in vswitchd/vswitch.xml
>>
>> + * make the timeout policy to be the default timeout policy. */
>> + int (*ct_add_timeout_policy)(struct dpif *, bool is_default,
>> + const struct ct_dpif_timeout_policy *tp);
>> + /* Gets a timeout policy and stores that into '*tp'.
>
>
>
>>
>> If 'is_default' is
>> + * true, sets '*tp' to the default timeout policy. Otherwise, gets the
>
>
> The above text does not make sense:
> "sets" ?
>
> "is_default" - can you explain this one ?
I re-wreite the comment as following.
/* Gets a timeout policy and stores that into '*tp'. If 'is_default' is
* true, gets default timeout policy. Otherwise, gets the timeout
* policy identified by 'tp_id'. */
int (*ct_get_timeout_policy)(struct dpif *, bool is_default,
uint32_t tp_id,
struct ct_dpif_timeout_policy *tp);
Thanks,
-Yi-Hung
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
