From: gilles@poolp.org
Subject: Re: smtpd: require an error message on table-proc failure replies
To: "Omar Polo" <op@omarpolo.com>, tech@openbsd.org
Date: Thu, 23 May 2024 16:41:07 +0000

May 22, 2024 7:43 PM, "Omar Polo" <op@omarpolo.com> wrote:

> 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 ok gilles@

for the readers, nothing makes use of error messages, they are essentially
translated to a TEMPFAIL but this gives an opportunity to let smtpd have a
mandatory logging of errors instead of relying on tables to maybe log.


> 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");