Hello,
Sorry for the long delay, I just missed the previous mail in the
backlog, I'm afraid.
Please don't feel bad to send a ping message if you don't hear a reply
in about a week, sometimes life just get in the way and the mails lost
in the backlog.
Philipp <phil...@bureaucracy.de> wrote:
> Hi
>
> [2024-05-30 21:54] Philipp <phil...@bureaucracy.de>
> > I'm currently working on the C API for the tables implementations. The
> > current API has the problem that the callbacks mixes request and
> > response. So I have created a new C api with callbacks for requests
> > and extra functios for response. The callbacks looks the following:
> >
> > void update_cb(const char *id, const char *tname)
> > void check_cb(const char *id, const char *tname, int service, const char
> > *key)
> > void lookup_cb(const char *id, const char *tname, int service, const char
> > *key)
> > void fetch_cb(const char *id, const char *tname, int service)
> >
> > These are called on for each request. The backend register these callbacks
> > and calles a response function:
> >
> > void table_api_update_finish(const char *id)
> > void table_api_check_result(const char *id, bool found)
> > void table_api_lookup_result(const char *id, const char *buf)
> > void table_api_lookup_finish(const char *id)
> > void table_api_fetch_result(const char *id, const char *buf)
> > table_api_error(const char *id, const char *error)
> >
> > In most cases the return value (found/not-found/ok) is implicit. For
> > lookup there are two functions for response, one for the value and one
> > to finish the lookup. This has two reasons: First this makes it simpler
> > to handle multible (i.e. for alias) responses. Secound this allows to
> > change the protokoll for more flexible lookup tables (usefull for dns
> > tables). The error function implicit respones with the correct result
> > type.
> >
> > What do you think about the new API?
Mostly looking good to me. I have a few nits and ideas on how we can
further simplify all of this so the table_stdio.c layer can remain very
simple.
> > I have also implement some fallback handler for the old api. They are
> > primary for simpler testing and development. Patcher are tested with
> > table-ldap and attached. The patches are currenty seperated by query
> > type. For the push I would put them togetter in one commit.
Actually the backward compat is not important. This is an internal
library used by a few tables, it's not published standalone and we don't
make any compatibility promise of any sort (ABI or API.)
It's fine to break the ABI/API if it makes sense, and I can help
migrating the existing code to the new one.
> > My next step is to change the getline loop to a pool based loop. Then
> > add some registration for other fd. This way the backend can be full
> > asyncron.
I think this is fine but table_stdio.c is not the right place to do so.
event loop are delicate, the application might need to schedule its own
events (at least postgres could query asynchronously the db IIRC) so I
prefer not to force the event loop in table_stdio.c. Not all tables
need to be asynchronous.
What table_stdio.c should be in my opinion is just a tiny layer to parse
a line into a struct or something equivalent and write the replies.
Once it does this, then writing both blocking and async tables on top
becomes way more easy.
> I now also have implemented a simple poll based event loop. So a table
> implementation can register fds with the following api:
>
> void table_api_register_fd(int fd, short events, fd_callback cb);
> void table_api_replace_fd(int old, int new);
> void table_api_remove_fd(int fd);
So this is what I'd avoid. Let's try to have each layer doing one thing
only (more or les), and leave the fd tracking and correlate events to
other layers.
I'd refactor the changes so that the APIs exported are something like
this, but mind you that this is just a suggestion, I haven't implemented
it
struct table_request {
enum table_operation op;
/* maybe even time_t timestamp; */
char *table_name;
enum table_service service;
char *id;
char *key;
};
/* parses the line into the provided storage for the struct */
int table_api_parse_request(struct table_request *, char *);
/* routines for the reply */
int table_api_update(struct table_request *, int);
int table_api_error(struct table_request *, const char *);
int table_api_found(struct table_request *, const char *);
int table_api_not_found(struct table_request *);
This could be the base layer, it just deal with parsing into caller
provided storage and fills the struct, then provides APIs to deal with
the reply. No dependencies on anything else other than the stdlib.
(after this, we can also add stuff to deal with multiple lookup result, I
really like your proposals wrt the protocol change, but we should try to
do only one thing at once ;-)
Making an async table over this is straightforward whichever event loop
you like the most.
Then I believe it's fine if we keep the register API implemented on top
of these for the common blocking case. The implementation can also be
straightforward.
I'd leave the choice of the event loop to use to the actual table
implementations. So that table-passwd can not care, and table-ldap can
use a poll-based loop or libevent or whatever it wants. Same for the
databases.
Running a fully asynchronous table on top of this with, say, libevent is
just a matter of a few lines of glue code, which is acceptable IMHO.
(i'm mentioning libevent because I've seen you're including event.h,
even if the event loop is actually based on poll.)
So, in the end, what I care the most is that we keep every layer as
simple and dependency-free as we can. I don't think the table_stdio
layer should keep track of inflight request, nor have its own event
loop, etc. It's the application problem, not matter of transport layer.
Let me know of what you think about this. Then, we can coordinate and
get it done. I have some free time this week so I can implement what I
was suggesting above if you like, so that you can focus on table_ldap.
Just let me know :)
Thanks,
Omar Polo
