Some comments inline.
> From 4a997162811d6b43a748af1cfb783bad6177dca8 Mon Sep 17 00:00:00 2001
> From: Philipp Takacs <[email protected]>
> 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:
