| Message ID | 1534272992-31756-8-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | conntrack zone limitation | expand |
> On Aug 14, 2018, at 11:56 AM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > +/* The caller takes ownership of 'struct ct_dpif_zone_limit *', and is > + * responsible to free that struct. */ > +struct ct_dpif_zone_limit * > +ct_dpif_pop_zone_limit(struct ovs_list *zone_limits) > +{ > + struct ct_dpif_zone_limit *zone_limit; > + LIST_FOR_EACH_POP (zone_limit, node, zone_limits) { > + return zone_limit; > + } > + OVS_NOT_REACHED(); > +} Aborting seems like a pretty serious action to take on calling this when it's not empty--especially if there's not a comment about it in the description. I think we would normally just return NULL if it's not empty. However, I think the function may not be needed since it's only used by ct_dpif_free_zone_limits() and could be simplified somewhat. What do you think of the incremental at the end? (I've only compile-tested it.) --Justin -=-=-=-=-=-=-=-=- diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index bb809d9920b5..d3e8c936d7e6 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -609,24 +609,14 @@ ct_dpif_push_zone_limit(struct ovs_list *zone_limits, uint16_t zone, ovs_list_push_back(zone_limits, &zone_limit->node); } -/* The caller takes ownership of 'struct ct_dpif_zone_limit *', and is - * responsible to free that struct. */ -struct ct_dpif_zone_limit * -ct_dpif_pop_zone_limit(struct ovs_list *zone_limits) -{ - struct ct_dpif_zone_limit *zone_limit; - LIST_FOR_EACH_POP (zone_limit, node, zone_limits) { - return zone_limit; - } - OVS_NOT_REACHED(); -} - void ct_dpif_free_zone_limits(struct ovs_list *zone_limits) { while (!ovs_list_is_empty(zone_limits)) { - struct ct_dpif_zone_limit *p = ct_dpif_pop_zone_limit(zone_limits); - free(p); + struct ovs_list *entry = ovs_list_pop_front(zone_limits); + struct ct_dpif_zone_limit* cdzl; + cdzl = CONTAINER_OF(entry, struct ct_dpif_zone_limit, node); + free(cdzl); } } diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index 71a1a2269522..decc14ffc2a0 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -221,7 +221,6 @@ void ct_dpif_format_tcp_stat(struct ds *, int, int); bool ct_dpif_parse_tuple(struct ct_dpif_tuple *, const char *s, struct ds *); void ct_dpif_push_zone_limit(struct ovs_list *, uint16_t zone, uint32_t limit, uint32_t count); -struct ct_dpif_zone_limit * ct_dpif_pop_zone_limit(struct ovs_list *); void ct_dpif_free_zone_limits(struct ovs_list *); bool ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone, uint32_t *plimit, struct ds *);
On Tue, Aug 14, 2018 at 4:29 PM Justin Pettit <jpettit@ovn.org> wrote: > > > > On Aug 14, 2018, at 11:56 AM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > > > +/* The caller takes ownership of 'struct ct_dpif_zone_limit *', and is > > + * responsible to free that struct. */ > > +struct ct_dpif_zone_limit * > > +ct_dpif_pop_zone_limit(struct ovs_list *zone_limits) > > +{ > > + struct ct_dpif_zone_limit *zone_limit; > > + LIST_FOR_EACH_POP (zone_limit, node, zone_limits) { > > + return zone_limit; > > + } > > + OVS_NOT_REACHED(); > > +} > > Aborting seems like a pretty serious action to take on calling this when it's not empty--especially if there's not a comment about it in the description. I think we would normally just return NULL if it's not empty. However, I think the function may not be needed since it's only used by ct_dpif_free_zone_limits() and could be simplified somewhat. > > What do you think of the incremental at the end? (I've only compile-tested it.) > > --Justin Thanks for the improvement. I think the changes make sense. I tested it with the system traffic test and it works fine. Thanks, -Yi-Hung
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index d1e8a6b8b4a9..a772799fe347 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -597,3 +597,35 @@ error: free(copy); return false; } + +void +ct_dpif_push_zone_limit(struct ovs_list *zone_limits, uint16_t zone, + uint32_t limit, uint32_t count) +{ + struct ct_dpif_zone_limit *zone_limit = xmalloc(sizeof *zone_limit); + zone_limit->zone = zone; + zone_limit->limit = limit; + zone_limit->count = count; + ovs_list_push_back(zone_limits, &zone_limit->node); +} + +/* The caller takes ownership of 'struct ct_dpif_zone_limit *', and is + * responsible to free that struct. */ +struct ct_dpif_zone_limit * +ct_dpif_pop_zone_limit(struct ovs_list *zone_limits) +{ + struct ct_dpif_zone_limit *zone_limit; + LIST_FOR_EACH_POP (zone_limit, node, zone_limits) { + return zone_limit; + } + OVS_NOT_REACHED(); +} + +void +ct_dpif_free_zone_limits(struct ovs_list *zone_limits) +{ + while (!ovs_list_is_empty(zone_limits)) { + struct ct_dpif_zone_limit *p = ct_dpif_pop_zone_limit(zone_limits); + free(p); + } +} diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index 4e83bc555e03..c80e18b72b56 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -219,5 +219,9 @@ void ct_dpif_format_tuple(struct ds *, const struct ct_dpif_tuple *); uint8_t ct_dpif_coalesce_tcp_state(uint8_t state); void ct_dpif_format_tcp_stat(struct ds *, int, int); bool ct_dpif_parse_tuple(struct ct_dpif_tuple *, const char *s, struct ds *); +void ct_dpif_push_zone_limit(struct ovs_list *, uint16_t zone, uint32_t limit, + uint32_t count); +struct ct_dpif_zone_limit * ct_dpif_pop_zone_limit(struct ovs_list *); +void ct_dpif_free_zone_limits(struct ovs_list *); #endif /* CT_DPIF_H */
This patch implments some helper function for conntrack zone limit. It will be useful for the following patches. Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- lib/ct-dpif.c | 32 ++++++++++++++++++++++++++++++++ lib/ct-dpif.h | 4 ++++ 2 files changed, 36 insertions(+)