Re-sending, I forgot to cc: aisha & tech:
> On 12 Jun 2021, at 22:47, Gilles CHEHADE <gil...@poolp.org> wrote: > >> >> On 12 Jun 2021, at 15:15, Eric Faurot <e...@openbsd.org> wrote: >> >> On Wed, Jun 09, 2021 at 05:41:36PM -0400, Aisha Tammy wrote: >>> Hi, >>> Here is the updated diff, which removes table_proc and adds table_procexec >>> as the default backend when no backend name matches. >>> >> >> Hi. >> >> I'm not opposed to the idea, but I have a couple of comments: >> >> First, if the two implementations are not going to coexists, >> we can just replace table_proc.c. > > True, though proc-exec was the name used for filters so it may be a good to > unify and drop the legacy “proc” name. > This will be hidden to users so quite frankly it’s a matter of dev preference. > > >> Second, since the goal is to expose the protocol directly, instead of >> relying on wrappers (through opensmtpd-extras api), we should take >> time to refine it properly before, and avoid having to bump it every >> other day. For example, I don't see the point of adding timestamps in >> the request. Do we want to be case-sensitive on the commands? Do we >> need some kind of handshake? Also, there can be long-term plan to use >> the same process for different tables, or even for other backends. > > I’m unsure I understand your point: > > The protocol is based on the filter protocol, follows the same logic and line > header to solve the same issues, this is precisely so that there’s no need to > bump every other day as we already figured what was needed for third party > adding to interoperate with smtpd. > This also has the advantage that you can have a single parser handle these > different API instead of having a filter protocol parser, a table protocol > parser (and maybe in the future a queue protocol parser… etc). > > WRT timestamps there were many uses for them ranging from timeout detection > both in daemon / add-ons, profiling, logging the time an event was generated > vs processes overhead, etc… > It allowed ensuring that all addons handling the same event have the exact > same timestamp associated to the event. > > WRT to case-sensitivity, I don’t recall using upper-case myself, to me the > protocol is case-sensitive and as far as I can remember I always used > lowercase :-/ > I’m all for lowering case here. > > WRT to handshake, it has multiple uses ranging from ensuring the addons get > their configuration from the daemon to synchronising the daemon start with > addons readiness. > Remember, we didn’t have this with filters and realised we couldn’t do > without, it is the same for tables, they need to get their “table name” at > start so we need the daemon to pass them, and we also need the daemon to not > start using them before they are ready. > > >> Finally the implementation could be factorized a bit, but that's a >> detail at this time. I think the close operation (is it really useful >> anyway?) >> should use fclose() instead of kill(), and maybe wait() too? > > The implementation can probably be improved, this was a PoC that allowed me > to write various table backends but it was never meant to be committed in the > first place.
