> On 12 Jun 2021, at 18:57, Aisha Tammy <openbsd.t...@aisha.cc> wrote:
>
> On 6/12/21 9:15 AM, Eric Faurot 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.
> Sounds good with me :D
>>
>> 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.
> My WIP LDAP wrapper does not use the timestamps. I have not been privy to the
> historical context of the development of this protocol, so I do not know why
> it exists.
> I am fine with removing it.
I’m unsure removing them would be a good idea, not only it wouldn’t bring any
benefit but it would prevent tracking the time an event was generated in the
daemon from a table backend and would make the protocol diverge from the
filters protocol while they are the same protocol as of today and could use the
same parser with different handler functions based on the event field.
>> Do we want to be case-sensitive on the commands?
> I reused the current table_service_name function present in table.c and hence
> set it to all-caps. I think keeping it such is not an issue but I don't care
> if we move to lowercase.
> I would prefer being case sensitive, so as not to overly-complicate our
> checks.
I agree.
>> Do we need some kind of handshake?
> I do not see a need for this between the table-open and the table-process.
> The table-process may do handshakes (or whatever) in its backend, which we
> should not be worrying about.
Maybe I misunderstood but the handshake is very much needed between the daemon
and non-builtin tables, similarly to filters.
This is what guarantees that the daemon doesn’t start using a table that’s not
ready but also what allows passing daemon informations to table backends so
they build their initial state.
>> Also, there can be long-term plan to use
>> the same process for different tables, or even for other backends.
>>
>> 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?
> I agree, I am not sure if the table-close is very useful. I am also fine with
> moving it to fclose, though I don't know how to manually close a table >.<
>
> BTW, can I remove the table_check function? I haven't seen a use for it even
> after scrounging around a bit.
>
> Unless any comments, I'll send the next patch (soonish) with fclose and
> timestamps, table_check removed.
>
