Hi Ben,
Your suspect is absolutely right, it’s a bug on 32-bit x86. This bug is related
to time_t struct, because time_t is a long int type.
See detail information about the code below:
————————————————————
static void
dpif_ipfix_exporter_init(struct dpif_ipfix_exporter *exporter)
{
exporter->collectors = NULL;
exporter->seq_number = 1;
exporter->last_template_set_time = TIME_MIN; ———> For 32-bit x86, TIME_MIN
is -2147483648, for 64-bit OS, TIME_MIN is -2^63
hmap_init(&exporter->cache_flow_key_map);
ovs_list_init(&exporter->cache_flow_start_timestamp_list);
exporter->cache_active_timeout = 0;
exporter->cache_max_flows = 0;
}
… …
if (!template_msg_sent
&& (exporter->last_template_set_time + IPFIX_TEMPLATE_INTERVAL)
<= export_time_sec) { ———> export_time_sec is signed int.
Because of type casting, ‘<=‘ will never be true on 32-bit x86 OS.
ipfix_send_template_msgs(exporter, export_time_sec,
entry->flow_key.obs_domain_id);
exporter->last_template_set_time = export_time_sec;
template_msg_sent = true;
}
————————————————————
Because of the analysis above, IPFIX template packet will never be sent on
32-bit x86 OS. That’s why the test case failed. I have addressed this
issue in patch v5. Do I need a separate patch to address this issue?
Bests,
Daniel Benli Ye
On Jun 10, 2016, at 11:39 AM, Ben Pfaff <b...@ovn.org<mailto:b...@ovn.org>>
wrote:
That was a fast revision! Thank you.
Even the new version fails for me, with the same difference: 10 packets
instead of 12. Do you have any ideas? The only unusual thing about my
system is that I use 32-bit x86 instead of 64-bit x86-64.
I'm appending a further incremental diff I suggest folding in, because I
noticed a few more style nits.
Thanks,
Ben.
On Fri, Jun 10, 2016 at 02:10:00AM +0000, Daniel Ye wrote:
Hi Ben,
Thanks for your review. For the test case, I didn’t get an error in my test
machine. I suspected that IPFIX packets are
not all sent before dump IPFIX stats, so 'sleep 1' is added to make sure all
the IPFIX packets are sent.
Bests,
Daniel Benli Ye
On Jun 10, 2016, at 5:33 AM, Ben Pfaff <b...@ovn.org<mailto:b...@ovn.org>>
wrote:
On Wed, Jun 08, 2016 at 06:45:25PM +0800, Benli Ye wrote:
It is meaningful for user to check the stats of IPFIX.
Using IPFIX stats, user can know how much flows the system
can support. It is also can be used for performance check
of IPFIX.
Thanks for posting v3!
I think that this is close. I suggest folding in the appended
incremental change.
I get test failures with this applied. I'm attaching them. Can you
take a look?
Thanks,
Ben.
diff --git a/ofproto/collectors.c b/ofproto/collectors.c
index 74d511d..57dc7ed 100644
--- a/ofproto/collectors.c
+++ b/ofproto/collectors.c
@@ -107,7 +107,7 @@ collectors_destroy(struct collectors *c)
size_t
collectors_send(const struct collectors *c, const void *payload, size_t n)
{
- uint16_t errors = 0;
+ size_t errors = 0;
if (c) {
size_t i;
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index eec71d7..0daa214 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -1359,13 +1359,13 @@ static void
ipfix_get_stats__(const struct dpif_ipfix_exporter *exporter,
ofproto_ipfix_stats *stats)
{
- memset(stats, 0xFF, sizeof *stats);
+ memset(stats, 0xff, sizeof *stats);
if (!exporter) {
return;
}
- *stats=exporter->stats;
+ *stats = exporter->stats;
}
static void
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 8e25942..93362cd 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1361,16 +1361,13 @@ struct ofproto_class {
const struct ofproto_ipfix_flow_exporter_options
*flow_exporters_options, size_t n_flow_exporters_options);
- /* Get IPFIX stats on 'ofproto' according to the exporter of birdge
+ /* Get IPFIX stats on 'ofproto' according to the exporter of bridge
* IPFIX or flow based IPFIX.
*
* OFPERR_NXST_NOT_CONFIGURED as a return value indicates that bridge
* IPFIX or flow based IPFIX is not configured. */
- int (*get_ipfix_stats)(
- const struct ofproto *ofproto,
- bool bridge_ipfix,
- struct ovs_list *replies
- );
+ int (*get_ipfix_stats)(const struct ofproto *ofproto,
+ bool bridge_ipfix, struct ovs_list *replies);
/* Configures connectivity fault management on 'ofport'.
*
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
