| Message ID | 1465610139-109725-1-git-send-email-daniely@vmware.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Acked-by: William Tu <u9012063@gmail.com> I think it fixes the issue, although the root cause is that we are 1) Comparing signed int (last_template_set_time) and unsigned int (export_time_sec). From the C99, the operand with signed integer type is converted to the type of the operand with unsigned integer type, so last_template_set_time is cast to unsigned int. ... && (exporter->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) <= export_time_sec) { And, after initial send, 2) assigning unsigned int to signed int exporter->last_template_set_time = export_time_sec; I don't have a 32-bit system to test, but explicitly cast to time_t should also solve the problem. Regards, William On Fri, Jun 10, 2016 at 6:55 PM, Benli Ye <daniely@vmware.com> wrote: > 'last_template_set_time' in truct dpif_ipfix_exporter is declared > as time_t and time_t is long int type. If we initialize > 'last_template_set_time' as TIME_MIN, whose value is -2147483648 > on 32-bit OS and -2^63 on 64-bit OS. There will be a problem on > 32-bit OS when comparing 'last_template_set_time' with a unisgned int > type variable, because type casting will happen and negative value > could be a large positive number. Fix this problem by simply initialize > 'last_template_set_time' as 0. > --- > ofproto/ofproto-dpif-ipfix.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c > index 79ba234..b1b2237 100644 > --- a/ofproto/ofproto-dpif-ipfix.c > +++ b/ofproto/ofproto-dpif-ipfix.c > @@ -495,7 +495,7 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter *exporter) > { > exporter->collectors = NULL; > exporter->seq_number = 1; > - exporter->last_template_set_time = TIME_MIN; > + exporter->last_template_set_time = 0; > hmap_init(&exporter->cache_flow_key_map); > ovs_list_init(&exporter->cache_flow_start_timestamp_list); > exporter->cache_active_timeout = 0; > @@ -511,7 +511,7 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter) > collectors_destroy(exporter->collectors); > exporter->collectors = NULL; > exporter->seq_number = 1; > - exporter->last_template_set_time = TIME_MIN; > + exporter->last_template_set_time = 0; > exporter->cache_active_timeout = 0; > exporter->cache_max_flows = 0; > } > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Sat, Jun 11, 2016 at 09:55:39AM +0800, Benli Ye wrote: > 'last_template_set_time' in truct dpif_ipfix_exporter is declared > as time_t and time_t is long int type. If we initialize > 'last_template_set_time' as TIME_MIN, whose value is -2147483648 > on 32-bit OS and -2^63 on 64-bit OS. There will be a problem on > 32-bit OS when comparing 'last_template_set_time' with a unisgned int > type variable, because type casting will happen and negative value > could be a large positive number. Fix this problem by simply initialize > 'last_template_set_time' as 0. I'm not really happy with the way that the ipfix module manages time, but this seems like the best "quick fix" for older branches especially. Can you provide a Signed-off-by?
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index 79ba234..b1b2237 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -495,7 +495,7 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter *exporter) { exporter->collectors = NULL; exporter->seq_number = 1; - exporter->last_template_set_time = TIME_MIN; + exporter->last_template_set_time = 0; hmap_init(&exporter->cache_flow_key_map); ovs_list_init(&exporter->cache_flow_start_timestamp_list); exporter->cache_active_timeout = 0; @@ -511,7 +511,7 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter) collectors_destroy(exporter->collectors); exporter->collectors = NULL; exporter->seq_number = 1; - exporter->last_template_set_time = TIME_MIN; + exporter->last_template_set_time = 0; exporter->cache_active_timeout = 0; exporter->cache_max_flows = 0; }