BUG/MINOR: tcp: Don't alter counters returned by tcp info fetchers
There are 2 kinds of tcp info fetchers. Those returning a time value (fc_rtt and
fc_rttval) and those returning a counter (fc_unacked, fc_sacked, fc_retrans,
fc_fackets, fc_lost, fc_reordering). Because of a bug, the counters were handled
as time values, and by default, were divided by 1000 (because of an invalid
conversion from us to ms). To work around this bug and have the right value, the
argument "us" had to be specified.
So now, tcp info fetchers returning a counter don't support any argument
anymore. To not break old configurations, if an argument is provided, it is
ignored and a warning is emitted during the configuration parsing.
In addition, parameter validiation is now performed during the configuration
parsing.
This patch must be backported as far as 1.7.
diff --git a/src/proto_tcp.c b/src/proto_tcp.c
index 25e01bd..3dfc6a6 100644
--- a/src/proto_tcp.c
+++ b/src/proto_tcp.c
@@ -1572,11 +1572,64 @@
#ifdef TCP_INFO
+
+/* Validates the arguments passed to "fc_*" fetch keywords returning a time
+ * value. These keywords support an optional string representing the unit of the
+ * result: "us" for microseconds and "ms" for milliseconds". Returns 0 on error
+ * and non-zero if OK.
+ */
+static int val_fc_time_value(struct arg *args, char **err)
+{
+ if (args[0].type == ARGT_STOP) {
+ args[0].type = ARGT_SINT;
+ args[0].data.sint = TIME_UNIT_MS;
+ }
+ if (args[0].type == ARGT_STR) {
+ if (strcmp(args[0].data.str.area, "us") == 0) {
+ free(args[0].data.str.area);
+ args[0].type = ARGT_SINT;
+ args[0].data.sint = TIME_UNIT_US;
+ }
+ else if (strcmp(args[0].data.str.area, "ms") == 0) {
+ free(args[0].data.str.area);
+ args[0].type = ARGT_SINT;
+ args[0].data.sint = TIME_UNIT_MS;
+ }
+ else {
+ memprintf(err, "expects 'us' or 'ms', got '%s'",
+ args[0].data.str.area);
+ return 0;
+ }
+ }
+ else {
+ memprintf(err, "Unexpected arg type");
+ return 0;
+ }
+
+ return 1;
+}
+
+/* Validates the arguments passed to "fc_*" fetch keywords returning a
+ * counter. These keywords should be used without any keyword, but because of a
+ * bug in previous versions, an optional string argument may be passed. In such
+ * case, the argument is ignored and a warning is emitted. Returns 0 on error
+ * and non-zero if OK.
+ */
+static int var_fc_counter(struct arg *args, char **err)
+{
+ if (args[0].type != ARGT_STOP) {
+ ha_warning("no argument supported for 'fc_*' sample expressions returning counters.\n");
+ if (args[0].type == ARGT_STR)
+ free(args[0].data.str.area);
+ args[0].type = ARGT_STOP;
+ }
+
+ return 1;
+}
+
/* Returns some tcp_info data if it's available. "dir" must be set to 0 if
* the client connection is required, otherwise it is set to 1. "val" represents
- * the required value. Use 0 for rtt and 1 for rttavg. "unit" is the expected unit
- * by default, the rtt is in us. Id "unit" is set to 0, the unit is us, if it is
- * set to 1, the units are milliseconds.
+ * the required value.
* If the function fails it returns 0, otherwise it returns 1 and "result" is filled.
*/
static inline int get_tcp_info(const struct arg *args, struct sample *smp,
@@ -1628,21 +1681,6 @@
default: return 0;
}
- /* Convert the value as expected. */
- if (args) {
- if (args[0].type == ARGT_STR) {
- if (strcmp(args[0].data.str.area, "us") == 0) {
- /* Do nothing. */
- } else if (strcmp(args[0].data.str.area, "ms") == 0) {
- smp->data.u.sint = (smp->data.u.sint + 500) / 1000;
- } else
- return 0;
- } else if (args[0].type == ARGT_STOP) {
- smp->data.u.sint = (smp->data.u.sint + 500) / 1000;
- } else
- return 0;
- }
-
return 1;
}
@@ -1652,6 +1690,11 @@
{
if (!get_tcp_info(args, smp, 0, 0))
return 0;
+
+ /* By default or if explicitly specified, convert rtt to ms */
+ if (!args || args[0].type == ARGT_STOP || args[0].data.sint == TIME_UNIT_MS)
+ smp->data.u.sint = (smp->data.u.sint + 500) / 1000;
+
return 1;
}
@@ -1661,6 +1704,11 @@
{
if (!get_tcp_info(args, smp, 0, 1))
return 0;
+
+ /* By default or if explicitly specified, convert rttvar to ms */
+ if (!args || args[0].type == ARGT_STOP || args[0].data.sint == TIME_UNIT_MS)
+ smp->data.u.sint = (smp->data.u.sint + 500) / 1000;
+
return 1;
}
@@ -1957,15 +2005,15 @@
{ "src_is_local", smp_fetch_src_is_local, 0, NULL, SMP_T_BOOL, SMP_USE_L4CLI },
{ "src_port", smp_fetch_sport, 0, NULL, SMP_T_SINT, SMP_USE_L4CLI },
#ifdef TCP_INFO
- { "fc_rtt", smp_fetch_fc_rtt, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
- { "fc_rttvar", smp_fetch_fc_rttvar, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_rtt", smp_fetch_fc_rtt, ARG1(0,STR), val_fc_time_value, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_rttvar", smp_fetch_fc_rttvar, ARG1(0,STR), val_fc_time_value, SMP_T_SINT, SMP_USE_L4CLI },
#if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__)
- { "fc_unacked", smp_fetch_fc_unacked, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
- { "fc_sacked", smp_fetch_fc_sacked, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
- { "fc_retrans", smp_fetch_fc_retrans, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
- { "fc_fackets", smp_fetch_fc_fackets, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
- { "fc_lost", smp_fetch_fc_lost, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
- { "fc_reordering", smp_fetch_fc_reordering, ARG1(0,STR), NULL, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_unacked", smp_fetch_fc_unacked, ARG1(0,STR), var_fc_counter, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_sacked", smp_fetch_fc_sacked, ARG1(0,STR), var_fc_counter, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_retrans", smp_fetch_fc_retrans, ARG1(0,STR), var_fc_counter, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_fackets", smp_fetch_fc_fackets, ARG1(0,STR), var_fc_counter, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_lost", smp_fetch_fc_lost, ARG1(0,STR), var_fc_counter, SMP_T_SINT, SMP_USE_L4CLI },
+ { "fc_reordering", smp_fetch_fc_reordering, ARG1(0,STR), var_fc_counter, SMP_T_SINT, SMP_USE_L4CLI },
#endif // linux || freebsd || netbsd
#endif // TCP_INFO
{ /* END */ },