Some comments inline. > From 4a997162811d6b43a748af1cfb783bad6177dca8 Mon Sep 17 00:00:00 2001 > From: Philipp Takacs <phil...@bureaucracy.de> > Date: Wed, 31 Jan 2024 17:50:52 +0100 > Subject: [PATCH 2/2] table-ldap handle more then one result > > --- > extras/tables/table-ldap/table_ldap.c | 98 +++++++++++++++++---------- > 1 file changed, 64 insertions(+), 34 deletions(-) > > diff --git a/extras/tables/table-ldap/table_ldap.c > b/extras/tables/table-ldap/table_ldap.c > index 3107d44..7f93fa6 100644 > --- a/extras/tables/table-ldap/table_ldap.c > +++ b/extras/tables/table-ldap/table_ldap.c > @@ -63,6 +63,10 @@ struct query { > int attrn; > }; > > +struct query_result { > + char **v[MAX_ATTRS]; > +};
There are too many level of pointers for my taste, and too much shared state passed around. For starters, why don't make this struct query_result more useful? It could be a container for the N results of the query, making the management more easy. Something like (with better naming) struct query_hit { char **v[MAX_ATTRS]; }; struct query_result { struct query_hit *res; size_t len; size_t cap; }; At that point we can declare query_result on the stack in ldap_run_query and pass around only one pointer instead of a two-level thingy. > static int ldap_run_query(int type, const char *, char *, size_t); > > static char *config, *url, *username, *password, *basedn, *ca_file; > @@ -397,12 +401,24 @@ table_ldap_lookup(int service, struct dict *params, > const char *key, char *dst, > } > > static int > -ldap_query(const char *filter, const char *key, char **attributes, char > ***outp, size_t n) > +realloc_results(struct query_result **r, size_t *num) > +{ > + struct query_result *new; > + size_t newsize = MAX(1, (*num)*2); > + if ((new = realloc(*r, newsize*sizeof(**r))) == NULL) > + return 0; > + *num = newsize; > + *r = new; > + return 1; > +} > + > +static int > +ldap_query(const char *filter, const char *key, char **attributes, struct > query_result **results, size_t n) > { > struct aldap_message *m = NULL; > struct aldap_page_control *pg = NULL; > - int ret, found; > - size_t i; > + int ret; > + size_t i, found = 0, numresults = 0; > char basedn__[MAX_LDAP_BASELEN]; > char filter__[MAX_LDAP_FILTERLEN]; > char key__[MAX_LDAP_IDENTIFIER]; > @@ -413,12 +429,11 @@ ldap_query(const char *filter, const char *key, char > **attributes, char ***outp, > return -1; > if (strlcpy(key__, key, sizeof key__) >= sizeof key__) > return -1; > - found = -1; > + ret = -1; I like this change. it's easier to reason about this stuff if we assume ret is -1 and set it to 0 or 1 only when we're about to return. > do { > - if ((ret = aldap_search(aldap, basedn__, LDAP_SCOPE_SUBTREE, > - filter__, key__, attributes, 0, 0, 0, pg)) == -1) { > - log_debug("ret=%d", ret); > - return -1; > + if (aldap_search(aldap, basedn__, LDAP_SCOPE_SUBTREE, > + filter__, key__, attributes, 0, 0, 0, pg) == -1) { > + goto error; > } > if (pg != NULL) { > aldap_freepage(pg); > @@ -433,26 +448,32 @@ ldap_query(const char *filter, const char *key, char > **attributes, char ***outp, > pg = m->page; > aldap_freemsg(m); > m = NULL; > - if (found == -1) > - found = 0; > + ret = 0; > break; > } > if (m->message_type != LDAP_RES_SEARCH_ENTRY) > goto error; > > - found = 1; > + if (found >= numresults) { > + if (!realloc_results(results, &numresults)) { > + goto error; > + } > + } > for (i = 0; i < n; ++i) > - if (aldap_match_attr(m, attributes[i], > &outp[i]) != 1) > + if (aldap_match_attr(m, attributes[i], > &(*results)[found].v[i]) != 1) > goto error; > aldap_freemsg(m); > m = NULL; > + found++; > } > } while (pg != NULL); > > - ret = found; > + if (found) > + ret = found; > goto end; > > error: > + free(*results); don't we need to aldap_free_attr() too? also, now this error label could go away entirely, we can just goto end and check there if ret == -1 and free the results. This can be done in a separate diff however, earlier or before this one, as you prefer. > ret = -1; > > end: