From: Omar Polo <op@omarpolo.com>
Subject: smtpd: require an error message on table-proc failure replies
To: tech@openbsd.org
Date: Wed, 22 May 2024 19:43:12 +0200

in retrospect I should have added a message for the `error' replies
since the beginning.  To ease the transition don't make it mandatory
until we fix the external tables and release an update, but document it
so that it's clear that it is required.

ok?

diff b1ba5f1878cd52d11c8259c6c0a082a1fef5733e 285b7278145d58779ed3b5e0267f9f935bf1679c
commit - b1ba5f1878cd52d11c8259c6c0a082a1fef5733e
commit + 285b7278145d58779ed3b5e0267f9f935bf1679c
blob - 2ecf72f23fb3782003d44d47ab220bc8bd54f67c
blob + c9761f0219c4399d0826dfbcb7f985f0f47ff758
--- usr.sbin/smtpd/smtpd-tables.7
+++ usr.sbin/smtpd/smtpd-tables.7
@@ -144,7 +144,7 @@ The result is either
 .Sq ok
 on success or
 .Sq error
-upon a failure to do so.
+and a message upon a failure to do so.
 .It Cm check Ar service id query
 Check whether
 .Ar query
@@ -155,7 +155,7 @@ if found,
 .Sq not-found
 if not, or
 .Sq error
-upon an error.
+and a message upon an error.
 .It Cm lookup Ar service id query
 Look up a value in the table for given the
 .Ar query .
@@ -165,7 +165,7 @@ and the value if found,
 .Sq not-found
 if not found, or
 .Sq error
-upon an error.
+and a message upon an error.
 .It Cm fetch Ar service id
 Fetch the next item from the table, eventually wrapping around.
 It is only supported for the
@@ -179,7 +179,7 @@ and the value if found,
 .Sq not-found
 if the table is empty, or
 .Sq error
-upon an error.
+and a message upon an error.
 .El
 .Pp
 Each service has a specific format for the result.
@@ -221,12 +221,12 @@ Used to map IP addresses to hostnames.
 .Sh EXAMPLES
 Assuming the table is called
 .Dq devs ,
-here's an example of a successful
+here's an example of a failed
 .Cm update
 transaction:
 .Bd -literal -offset indent
 table|0.1|1713795097.394049|devs|update|478ff0d2
-update-result|478ff0d2|ok
+update-result|478ff0d2|error|failed to connect to the database
 .Ed
 .Pp
 A
blob - 52cd505fa056bb97bba9409e9304e34880490379
blob + 8e5b6d9fa4eaed0c05f296907536ef3d52aa1863
--- usr.sbin/smtpd/table_proc.c
+++ usr.sbin/smtpd/table_proc.c
@@ -186,8 +186,15 @@ table_proc_update(struct table *table)
 	r = table_proc_recv(table, "update-result");
 	if (!strcmp(r, "ok"))
 		return (1);
-	if (!strcmp(r, "error"))
+
+	if (!strncmp(r, "error", 5)) {
+		if (r[5] == '|') {
+			r += 6;
+			log_warnx("warn: table-proc: %s update failed: %s",
+			    table->t_name, r);
+		}
 		return (0);
+	}
 
 	log_warnx("warn: table-proc: failed parse reply");
 	fatalx("table-proc: exiting");
@@ -222,11 +229,17 @@ table_proc_lookup(struct table *table, enum table_serv
 	table_proc_send(table, req, s, k);
 	r = table_proc_recv(table, res);
 
-	/* common replies */
 	if (!strcmp(r, "not-found"))
 		return (0);
-	if (!strcmp(r, "error"))
+
+	if (!strncmp(r, "error", 5)) {
+		if (r[5] == '|') {
+			r += 6;
+			log_warnx("warn: table-proc: %s %s failed: %s",
+			    table->t_name, req, r);
+		}
 		return (-1);
+	}
 
 	if (dst == NULL) {
 		/* check op */
@@ -261,8 +274,15 @@ table_proc_fetch(struct table *table, enum table_servi
 
 	if (!strcmp(r, "not-found"))
 		return (0);
-	if (!strcmp(r, "error"))
+
+	if (!strncmp(r, "error", 5)) {
+		if (r[5] == '|') {
+			r += 6;
+			log_warnx("warn: table-proc: %s fetch failed: %s",
+			    table->t_name, r);
+		}
 		return (-1);
+	}
 
 	if (strncmp(r, "found|", 6) != 0) {
 		log_warnx("warn: table-proc: failed to parse reply");