From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Tue, 30 Apr 2024 13:52:57 +0000 (+0200)
Subject: MEDIUM: log: optimizing tmp->type handling in sess_build_logline()
X-Git-Tag: v3.0-dev10~6
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=48e0efb00bd5a81c6f093f7d7561888e7ef2c44d;p=thirdparty%2Fhaproxy.git

MEDIUM: log: optimizing tmp->type handling in sess_build_logline()

Instead of chaining 2 switchcases and performing encoding checks for all
nodes let's actually split the logic in 2: first handle simple node types
(text/separator), and then handle dynamic node types (tag, expr). Encoding
options are only evaluated for dynamic node types.

Also, last_isspace is always set to 0 after next_fmt label, since next_fmt
label is only used for dynamic nodes, thus != LOG_FMT_SEPARATOR.

Since LF_NODE_WITH_OPT() macro (which was introduced recently) is now
unused, let's get rid of it.

No functional change should be expected.

(Use diff -w to check patch changes since reindentation makes the patch
look heavy, but in fact it remains fairly small)
---

diff --git a/include/haproxy/log-t.h b/include/haproxy/log-t.h
index e3a3e8f41b..0f684f4301 100644
--- a/include/haproxy/log-t.h
+++ b/include/haproxy/log-t.h
@@ -174,10 +174,6 @@ struct logformat_node {
 	const struct logformat_tag *tag; // set if ->type == LOG_FMT_TAG
 };
 
-/* returns true if the node options may be set (according to it's type) */
-#define LF_NODE_WITH_OPT(node) \
-  (node->type == LOG_FMT_EXPR || node->type == LOG_FMT_TAG)
-
 enum lf_expr_flags {
 	LF_FL_NONE     = 0x00,
 	LF_FL_COMPILED = 0x01
diff --git a/src/log.c b/src/log.c
index 098d16867a..f6ab921c20 100644
--- a/src/log.c
+++ b/src/log.c
@@ -3644,16 +3644,43 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
 		struct sample *key;
 		const struct buffer empty = { };
 
+		/* first start with basic types (use continue statement to skip
+		 * the current node)
+		 */
+		if (tmp->type == LOG_FMT_SEPARATOR) {
+			if (g_options & LOG_OPT_ENCODE) {
+				/* ignored when global encoding is set */
+				continue;
+			}
+			if (!last_isspace) {
+				LOGCHAR(' ');
+				last_isspace = 1;
+			}
+			continue;
+		}
+		else if (tmp->type == LOG_FMT_TEXT) {
+			/* text */
+			if (g_options & LOG_OPT_ENCODE) {
+				/* ignored when global encoding is set */
+				continue;
+			}
+			src = tmp->arg;
+			iret = strlcpy2(tmplog, src, dst + maxsize - tmplog);
+			if (iret == 0)
+				goto out;
+			tmplog += iret;
+			last_isspace = 0; /* data was written */
+			continue;
+		}
+
+		/* dynamic types handling (use "goto next_fmt" statement to skip
+		 * the current node)
+		 */
+
 		if (g_options & LOG_OPT_ENCODE) {
 			/* only consider global ctx for key encoding */
 			lf_buildctx_prepare(&ctx, g_options, NULL);
 
-			/* types that cannot be named such as text or separator are ignored
-			 * when encoding is set
-			 */
-			if (!LF_NODE_WITH_OPT(tmp))
-				goto next_fmt;
-
 			if (!tmp->name)
 				goto next_fmt; /* cannot represent anonymous field, ignore */
 
@@ -3692,107 +3719,88 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
 		 */
 		lf_buildctx_prepare(&ctx, g_options, tmp);
 
-		switch (tmp->type) {
-			case LOG_FMT_SEPARATOR:
-				if (!last_isspace) {
-					LOGCHAR(' ');
-					last_isspace = 1;
-				}
-				break;
+		if (tmp->type == LOG_FMT_EXPR) {
+			/* sample expression, may be request or response */
+			int type;
 
-			case LOG_FMT_TEXT: // text
-				src = tmp->arg;
-				iret = strlcpy2(tmplog, src, dst + maxsize - tmplog);
-				if (iret == 0)
-					goto out;
-				tmplog += iret;
-				break;
+			key = NULL;
+			if (ctx.options & LOG_OPT_REQ_CAP)
+				key = sample_process(be, sess, s, SMP_OPT_DIR_REQ|SMP_OPT_FINAL, tmp->expr, NULL);
 
-			case LOG_FMT_EXPR: // sample expression, may be request or response
-			{
-				int type;
+			if (!key && (ctx.options & LOG_OPT_RES_CAP))
+				key = sample_process(be, sess, s, SMP_OPT_DIR_RES|SMP_OPT_FINAL, tmp->expr, NULL);
 
-				key = NULL;
-				if (ctx.options & LOG_OPT_REQ_CAP)
-					key = sample_process(be, sess, s, SMP_OPT_DIR_REQ|SMP_OPT_FINAL, tmp->expr, NULL);
+			if (!key && !(ctx.options & (LOG_OPT_REQ_CAP|LOG_OPT_RES_CAP))) // cfg, cli
+				key = sample_process(be, sess, s, SMP_OPT_FINAL, tmp->expr, NULL);
 
-				if (!key && (ctx.options & LOG_OPT_RES_CAP))
-					key = sample_process(be, sess, s, SMP_OPT_DIR_RES|SMP_OPT_FINAL, tmp->expr, NULL);
+			type = SMP_T_STR; // default
 
-				if (!key && !(ctx.options & (LOG_OPT_REQ_CAP|LOG_OPT_RES_CAP))) // cfg, cli
-					key = sample_process(be, sess, s, SMP_OPT_FINAL, tmp->expr, NULL);
-
-				type = SMP_T_STR; // default
+			if (key && key->data.type == SMP_T_BIN &&
+			    (ctx.options & LOG_OPT_BIN)) {
+				/* output type is binary, and binary option is set:
+				 * preserve output type unless typecast is set to
+				 * force output type to string
+				 */
+				if (ctx.typecast != SMP_T_STR)
+					type = SMP_T_BIN;
+			}
 
-				if (key && key->data.type == SMP_T_BIN &&
-				    (ctx.options & LOG_OPT_BIN)) {
-					/* output type is binary, and binary option is set:
-					 * preserve output type unless typecast is set to
-					 * force output type to string
-					 */
-					if (ctx.typecast != SMP_T_STR)
-						type = SMP_T_BIN;
+			/* if encoding is set, try to preserve output type
+			 * with respect to typecast settings
+			 * (ie: str, sint, bool)
+			 *
+			 * Special case for cbor encoding: we also try to
+			 * preserve bin output type since cbor encoders
+			 * know how to deal with binary data.
+			 */
+			if (ctx.options & LOG_OPT_ENCODE) {
+				if (ctx.typecast == SMP_T_STR ||
+				    ctx.typecast == SMP_T_SINT ||
+				    ctx.typecast == SMP_T_BOOL) {
+					/* enforce type */
+					type = ctx.typecast;
 				}
-
-				/* if encoding is set, try to preserve output type
-				 * with respect to typecast settings
-				 * (ie: str, sint, bool)
-				 *
-				 * Special case for cbor encoding: we also try to
-				 * preserve bin output type since cbor encoders
-				 * know how to deal with binary data.
-				 */
-				if (ctx.options & LOG_OPT_ENCODE) {
-					if (ctx.typecast == SMP_T_STR ||
-					    ctx.typecast == SMP_T_SINT ||
-					    ctx.typecast == SMP_T_BOOL) {
-						/* enforce type */
-						type = ctx.typecast;
-					}
-					else if (key &&
-					         (key->data.type == SMP_T_SINT ||
-					          key->data.type == SMP_T_BOOL ||
-					          ((ctx.options & LOG_OPT_ENCODE_CBOR) &&
-					           key->data.type == SMP_T_BIN))) {
-						/* preserve type */
-						type = key->data.type;
-					}
+				else if (key &&
+				         (key->data.type == SMP_T_SINT ||
+				          key->data.type == SMP_T_BOOL ||
+				          ((ctx.options & LOG_OPT_ENCODE_CBOR) &&
+				           key->data.type == SMP_T_BIN))) {
+					/* preserve type */
+					type = key->data.type;
 				}
+			}
 
-				if (key && !sample_convert(key, type))
-					key = NULL;
-
-				if (ctx.options & LOG_OPT_HTTP)
+			if (key && !sample_convert(key, type))
+				key = NULL;
+			if (ctx.options & LOG_OPT_HTTP)
+				ret = lf_encode_chunk(tmplog, dst + maxsize,
+				                      '%', http_encode_map, key ? &key->data.u.str : &empty, &ctx);
+			else {
+				if (key && type == SMP_T_BIN)
 					ret = lf_encode_chunk(tmplog, dst + maxsize,
-					                      '%', http_encode_map, key ? &key->data.u.str : &empty, &ctx);
-				else {
-					if (key && type == SMP_T_BIN)
-						ret = lf_encode_chunk(tmplog, dst + maxsize,
-						                      0, no_escape_map,
-						                      &key->data.u.str,
-						                      &ctx);
-					else if (key && type == SMP_T_SINT)
-						ret = lf_int_encode(tmplog, dst + maxsize - tmplog,
-						                    key->data.u.sint, &ctx);
-					else if (key && type == SMP_T_BOOL)
-						ret = lf_bool_encode(tmplog, dst + maxsize - tmplog,
-						                     key->data.u.sint, &ctx);
-					else
-						ret = lf_text_len(tmplog,
-						                  key ? key->data.u.str.area : NULL,
-						                  key ? key->data.u.str.data : 0,
-						                  dst + maxsize - tmplog,
-						                  &ctx);
-				}
-				if (ret == NULL)
-					goto out;
-				tmplog = ret;
-				break;
+					                      0, no_escape_map,
+					                      &key->data.u.str,
+					                      &ctx);
+				else if (key && type == SMP_T_SINT)
+					ret = lf_int_encode(tmplog, dst + maxsize - tmplog,
+					                    key->data.u.sint, &ctx);
+				else if (key && type == SMP_T_BOOL)
+					ret = lf_bool_encode(tmplog, dst + maxsize - tmplog,
+					                     key->data.u.sint, &ctx);
+				else
+					ret = lf_text_len(tmplog,
+					                  key ? key->data.u.str.area : NULL,
+					                  key ? key->data.u.str.data : 0,
+					                  dst + maxsize - tmplog,
+					                  &ctx);
 			}
+			if (ret == NULL)
+				goto out;
+			tmplog = ret;
+			goto next_fmt;
 		}
 
-		if (tmp->type != LOG_FMT_TAG)
-			goto next_fmt;
+		BUG_ON(tmp->type != LOG_FMT_TAG);
 
 		/* logformat tag */
 		switch (tmp->tag->type) {
@@ -4748,8 +4756,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
 
 		}
  next_fmt:
-		if (tmp->type != LOG_FMT_SEPARATOR)
-			last_isspace = 0; // not a separator, hence not a space
+		last_isspace = 0;
 
 		if (value_beg == tmplog) {
 			/* handle the case where no data was generated for the value after