MINOR: server: srv_append_status refacto

srv_append_status() has become a swiss-knife function over time.
It is used from server code and also from checks code, with various
inputs and distincts code paths, making it very hard to guess the
actual behavior of the function (resulting string output).

To simplify the logic behind it, we're dividing it in multiple contextual
functions that take simple inputs and do explicit things, making them
more predictable and easier to maintain.

diff --git a/include/haproxy/check.h b/include/haproxy/check.h
index 485c494935a52ee06c6221c687bd8a83191db6ac..c90d3e77fc19da077b89812d6c1639f2e611c787 100644 (file)
--- a/include/haproxy/check.h
+++ b/include/haproxy/check.h
@@ -67,6 +67,7 @@ const char *get_check_status_description(short check_status);
 const char *get_check_status_info(short check_status);
 int httpchk_build_status_header(struct server *s, struct buffer *buf);
 void __health_adjust(struct server *s, short status);
+void check_append_info(struct buffer *msg, struct check *check);
 void set_server_check_status(struct check *check, short status, const char *desc);
 void chk_report_conn_err(struct check *check, int errno_bck, int expired);
 void check_notify_failure(struct check *check);

diff --git a/src/check.c b/src/check.c
index f39f88dd941504b5685a327adc81fe607b08ec65..2ff3bdb695e9b6ad0ceed342da852b360a8c954c 100644 (file)
--- a/src/check.c
+++ b/src/check.c
@@ -428,6 +428,31 @@ const char *get_analyze_status(short analyze_status) {
                return analyze_statuses[HANA_STATUS_UNKNOWN].desc;
 }
 
+/* append check info to buffer msg */
+void check_append_info(struct buffer *msg, struct check *check)
+{
+       if (!check)
+               return;
+       chunk_appendf(msg, ", reason: %s", get_check_status_description(check->status));
+
+       if (check->status >= HCHK_STATUS_L57DATA)
+               chunk_appendf(msg, ", code: %d", check->code);
+
+       if (check->desc[0]) {
+               struct buffer src;
+
+               chunk_appendf(msg, ", info: \"");
+
+               chunk_initlen(&src, check->desc, 0, strlen(check->desc));
+               chunk_asciiencode(msg, &src, '"');
+
+               chunk_appendf(msg, "\"");
+       }
+
+       if (check->duration >= 0)
+               chunk_appendf(msg, ", check duration: %ldms", check->duration);
+}
+
 /* Sets check->status, update check->duration and fill check->result with an
  * adequate CHK_RES_* value. The new check->health is computed based on the
  * result.
@@ -528,7 +553,7 @@ void set_server_check_status(struct check *check, short status, const char *desc
                             (check->result == CHK_RES_CONDPASS) ? "conditionally ":"",
                             (check->result >= CHK_RES_PASSED)   ? "succeeded" : "failed");
 
-               srv_append_status(&trash, s, check, -1, 0);
+               check_append_info(&trash, check);
 
                chunk_appendf(&trash, ", status: %d/%d %s",
                             (check->health >= check->rise) ? check->health - check->rise + 1 : check->health,

diff --git a/src/server.c b/src/server.c
index 9483e8972fa1c13e3b7c196b1a7ee1465823f5f8..501657b47967bcefd7102ae4290441f48e900578 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -1511,57 +1511,33 @@ void srv_shutdown_backup_streams(struct proxy *px, int why)
                        srv_shutdown_streams(srv, why);
 }
 
-/* Appends some information to a message string related to a server going UP or
- * DOWN.  If both <forced> and <reason> are null and the server tracks another
- * one, a "via" information will be provided to know where the status came from.
- * If <check> is non-null, an entire string describing the check result will be
- * appended after a comma and a space (eg: to report some information from the
- * check that changed the state). In the other case, the string will be built
- * using the check results stored into the struct server if present.
+static void srv_append_op_chg_cause(struct buffer *msg, struct server *s, struct check *check)
+{
+       if (check)
+               check_append_info(msg, check);
+       else if (s->op_st_chg.reason && *s->op_st_chg.reason)
+               chunk_appendf(msg, ", %s", s->op_st_chg.reason);
+}
+
+static void srv_append_adm_chg_cause(struct buffer *msg, struct server *s)
+{
+       if (s->adm_st_chg_cause)
+               chunk_appendf(msg, " (%s)", srv_adm_st_chg_cause(s->adm_st_chg_cause));
+}
+
+/* Appends some information to a message string related to a server tracking
+ * or requeued connections info.
+ *
+ * If <forced> is null and the server tracks another one, a "via"
  * If <xferred> is non-negative, some information about requeued sessions are
  * provided.
  *
  * Must be called with the server lock held.
  */
-void srv_append_status(struct buffer *msg, struct server *s,
-                      struct check *check, int xferred, int forced)
+static void srv_append_more(struct buffer *msg, struct server *s,
+                            int xferred, int forced)
 {
-       short status = s->op_st_chg.status;
-       short code = s->op_st_chg.code;
-       long duration = s->op_st_chg.duration;
-       char *desc = s->op_st_chg.reason;
-
-       if (check) {
-               status = check->status;
-               code = check->code;
-               duration = check->duration;
-               desc = check->desc;
-       }
-
-       if (status != -1) {
-               chunk_appendf(msg, ", reason: %s", get_check_status_description(status));
-
-               if (status >= HCHK_STATUS_L57DATA)
-                       chunk_appendf(msg, ", code: %d", code);
-
-               if (desc && *desc) {
-                       struct buffer src;
-
-                       chunk_appendf(msg, ", info: \"");
-
-                       chunk_initlen(&src, desc, 0, strlen(desc));
-                       chunk_asciiencode(msg, &src, '"');
-
-                       chunk_appendf(msg, "\"");
-               }
-
-               if (duration >= 0)
-                       chunk_appendf(msg, ", check duration: %ldms", duration);
-       }
-        else if (desc && *desc) {
-                chunk_appendf(msg, ", %s", desc);
-        }
-       else if (!forced && s->track) {
+       if (!forced && s->track) {
                chunk_appendf(msg, " via %s/%s", s->track->proxy->id, s->track->id);
        }
 
@@ -5354,7 +5330,9 @@ static void srv_update_status(struct server *s)
                                             "%sServer %s/%s is DOWN", s->flags & SRV_F_BACKUP ? "Backup " : "",
                                             s->proxy->id, s->id);
 
-                               srv_append_status(tmptrash, s, NULL, xferred, 0);
+                               srv_append_op_chg_cause(tmptrash, s, NULL);
+                               srv_append_more(tmptrash, s, xferred, 0);
+
                                ha_warning("%s.\n", tmptrash->area);
 
                                /* we don't send an alert if the server was previously paused */
@@ -5382,7 +5360,8 @@ static void srv_update_status(struct server *s)
                                             "%sServer %s/%s is stopping", s->flags & SRV_F_BACKUP ? "Backup " : "",
                                             s->proxy->id, s->id);
 
-                               srv_append_status(tmptrash, s, NULL, xferred, 0);
+                               srv_append_op_chg_cause(tmptrash, s, NULL);
+                               srv_append_more(tmptrash, s, xferred, 0);
 
                                ha_warning("%s.\n", tmptrash->area);
                                send_log(s->proxy, LOG_NOTICE, "%s.\n",
@@ -5424,7 +5403,9 @@ static void srv_update_status(struct server *s)
                                             "%sServer %s/%s is UP", s->flags & SRV_F_BACKUP ? "Backup " : "",
                                             s->proxy->id, s->id);
 
-                               srv_append_status(tmptrash, s, NULL, xferred, 0);
+                               srv_append_op_chg_cause(tmptrash, s, NULL);
+                               srv_append_more(tmptrash, s, xferred, 0);
+
                                ha_warning("%s.\n", tmptrash->area);
                                send_log(s->proxy, LOG_NOTICE, "%s.\n",
                                         tmptrash->area);
@@ -5460,13 +5441,10 @@ static void srv_update_status(struct server *s)
                        tmptrash = alloc_trash_chunk();
                        if (tmptrash) {
                                chunk_printf(tmptrash,
-                                   "%sServer %s/%s was DOWN and now enters maintenance%s%s%s",
-                                   s->flags & SRV_F_BACKUP ? "Backup " : "", s->proxy->id, s->id,
-                                   (s->adm_st_chg_cause) ? " (" : "",
-                                   srv_adm_st_chg_cause(s->adm_st_chg_cause),
-                                   (s->adm_st_chg_cause) ? ")" : "");
-
-                               srv_append_status(tmptrash, s, NULL, -1, (s->next_admin & SRV_ADMF_FMAINT));
+                                   "%sServer %s/%s was DOWN and now enters maintenance",
+                                   s->flags & SRV_F_BACKUP ? "Backup " : "", s->proxy->id, s->id);
+                               srv_append_adm_chg_cause(tmptrash, s);
+                               srv_append_more(tmptrash, s, -1, (s->next_admin & SRV_ADMF_FMAINT));
 
                                if (!(global.mode & MODE_STARTING)) {
                                        ha_warning("%s.\n", tmptrash->area);
@@ -5500,14 +5478,11 @@ static void srv_update_status(struct server *s)
                        tmptrash = alloc_trash_chunk();
                        if (tmptrash) {
                                chunk_printf(tmptrash,
-                                            "%sServer %s/%s is going DOWN for maintenance%s%s%s",
+                                            "%sServer %s/%s is going DOWN for maintenance",
                                             s->flags & SRV_F_BACKUP ? "Backup " : "",
-                                            s->proxy->id, s->id,
-                                            (s->adm_st_chg_cause) ? " (" : "",
-                                            srv_adm_st_chg_cause(s->adm_st_chg_cause),
-                                            (s->adm_st_chg_cause) ? ")" : "");
-
-                               srv_append_status(tmptrash, s, NULL, xferred, (s->next_admin & SRV_ADMF_FMAINT));
+                                            s->proxy->id, s->id);
+                               srv_append_adm_chg_cause(tmptrash, s);
+                               srv_append_more(tmptrash, s, xferred, (s->next_admin & SRV_ADMF_FMAINT));
 
                                if (!(global.mode & MODE_STARTING)) {
                                        ha_warning("%s.\n", tmptrash->area);
@@ -5686,13 +5661,10 @@ static void srv_update_status(struct server *s)
 
                        tmptrash = alloc_trash_chunk();
                        if (tmptrash) {
-                               chunk_printf(tmptrash, "%sServer %s/%s enters drain state%s%s%s",
-                                            s->flags & SRV_F_BACKUP ? "Backup " : "", s->proxy->id, s->id,
-                                            (s->adm_st_chg_cause) ? " (" : "",
-                                            srv_adm_st_chg_cause(s->adm_st_chg_cause),
-                                            (s->adm_st_chg_cause) ? ")" : "");
-
-                               srv_append_status(tmptrash, s, NULL, xferred, (s->next_admin & SRV_ADMF_FDRAIN));
+                               chunk_printf(tmptrash, "%sServer %s/%s enters drain state",
+                                            s->flags & SRV_F_BACKUP ? "Backup " : "", s->proxy->id, s->id);
+                               srv_append_adm_chg_cause(tmptrash, s);
+                               srv_append_more(tmptrash, s, xferred, (s->next_admin & SRV_ADMF_FDRAIN));
 
                                if (!(global.mode & MODE_STARTING)) {
                                        ha_warning("%s.\n", tmptrash->area);