[2024-02-04 10:50] Omar Polo <o...@omarpolo.com>
> 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.
Yes this had been also a bit my problem, I belive I found now a sane
solution. Pass a pointer-pointer to ldap_query() and set the pointer
when there is no error.
> 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.
I understand the idea, but im not convicend it is currently usefull.
Because currently the result is only allocated in ldap_query and
used/freed in ldap_run_query. So putting the len and cap variable in
a struct only adds a level of indirection without a benetfit.
> > 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.
Problem is, there is still a bug in there. ret must be set to -1 at each
itteration of the do-while loop. A fixed version is attached.
But in general this make the code more readable.
> > 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?
Yes, I forgott the aldap_free_attr().
> 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.
I have attached a patch for this.
Philipp
From c75c608a764213a1fa691a7b1f1ab9f3b98f2627 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/3] table-ldap handle more then one result
---
extras/tables/table-ldap/table_ldap.c | 105 ++++++++++++++++++--------
1 file changed, 72 insertions(+), 33 deletions(-)
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 3107d44..354e09d 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];
+};
+
static int ldap_run_query(int type, const char *, char *, size_t);
static char *config, *url, *username, *password, *basedn, *ca_file;
@@ -397,12 +401,25 @@ 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;
+ struct query_result *res = NULL;
+ int ret;
+ size_t i, j, found = 0, numresults = 0;
char basedn__[MAX_LDAP_BASELEN];
char filter__[MAX_LDAP_FILTERLEN];
char key__[MAX_LDAP_IDENTIFIER];
@@ -413,12 +430,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;
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;
+ ret = -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 +449,40 @@ 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(&res, &numresults)) {
+ goto error;
+ }
+ }
+ memset(&res[found], 0, sizeof(res[found]));
for (i = 0; i < n; ++i)
- if (aldap_match_attr(m, attributes[i], &outp[i]) != 1)
+ if (aldap_match_attr(m, attributes[i], &res[found].v[i]) != 1)
goto error;
aldap_freemsg(m);
m = NULL;
+ found++;
}
} while (pg != NULL);
+ if (ret == -1)
+ goto error;
ret = found;
+ *results = res;
goto end;
error:
+ for (i = 0; i < found; i++) {
+ for (j = 0; j < n; j++) {
+ aldap_free_attr(res[i].v[j]);
+ }
+ }
+ free(res);
ret = -1;
end:
@@ -465,9 +495,10 @@ end:
static int
ldap_run_query(int type, const char *key, char *dst, size_t sz)
{
- struct query *q;
- char **res[4], filter[MAX_LDAP_FILTERLEN];
- int ret, i;
+ struct query *q;
+ struct query_result *res = NULL;
+ char filter[MAX_LDAP_FILTERLEN];
+ int ret, i, j;
switch (type) {
case K_ALIAS: q = &queries[LDAP_ALIAS]; break;
@@ -495,39 +526,40 @@ ldap_run_query(int type, const char *key, char *dst, size_t sz)
return -1;
}
- memset(res, 0, sizeof(res));
- ret = ldap_query(filter, key, q->attrs, res, q->attrn);
+ ret = ldap_query(filter, key, q->attrs, &res, q->attrn);
if (ret <= 0 || dst == NULL)
goto end;
switch (type) {
case K_ALIAS:
+ case K_MAILADDRMAP:
memset(dst, 0, sz);
- for (i = 0; res[0][i]; i++) {
- if (i && strlcat(dst, ", ", sz) >= sz) {
- ret = -1;
- break;
- }
- if (strlcat(dst, res[0][i], sz) >= sz) {
- ret = -1;
- break;
+ for (j = 0; j < ret; j++) {
+ for (i = 0; res[j].v[0][i]; i++) {
+ if ((i || j) && strlcat(dst, ", ", sz) >= sz) {
+ ret = -1;
+ break;
+ }
+ if (strlcat(dst, res[j].v[0][i], sz) >= sz) {
+ ret = -1;
+ break;
+ }
}
}
break;
case K_DOMAIN:
case K_MAILADDR:
- case K_MAILADDRMAP:
- if (strlcpy(dst, res[0][0], sz) >= sz)
+ if (strlcpy(dst, res[0].v[0][0], sz) >= sz)
ret = -1;
break;
case K_CREDENTIALS:
- if (snprintf(dst, sz, "%s:%s", res[0][0], res[1][0]) >= (int)sz)
+ if (snprintf(dst, sz, "%s:%s", res[0].v[0][0], res[0].v[1][0]) >= (int)sz)
ret = -1;
break;
case K_USERINFO:
- if (snprintf(dst, sz, "%s:%s:%s", res[0][0], res[1][0],
- res[2][0]) >= (int)sz)
+ if (snprintf(dst, sz, "%s:%s:%s", res[0].v[0][0], res[0].v[1][0],
+ res[0].v[2][0]) >= (int)sz)
ret = -1;
break;
default:
@@ -539,10 +571,17 @@ ldap_run_query(int type, const char *key, char *dst, size_t sz)
log_warnx("warn: could not format result");
end:
- for (i = 0; i < q->attrn; ++i)
- if (res[i])
- aldap_free_attr(res[i]);
+ for (j = 0; j < ret; ++j) {
+ for (i = 0; i < q->attrn; ++i) {
+ if (res[j].v[i]) {
+ aldap_free_attr(res[j].v[i]);
+ }
+ }
+ }
+ free(res);
+ if (ret > 0)
+ ret = 1;
return ret;
}
--
2.39.2
From d1ec83464689a1b927bf08a5de758c54ab04f688 Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Sun, 4 Feb 2024 15:22:58 +0100
Subject: [PATCH 3/3] table-ldap make error handle more readable
---
extras/tables/table-ldap/table_ldap.c | 34 ++++++++++++---------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 354e09d..8b2ffa9 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -434,7 +434,7 @@ ldap_query(const char *filter, const char *key, char **attributes, struct query_
ret = -1;
if (aldap_search(aldap, basedn__, LDAP_SCOPE_SUBTREE,
filter__, key__, attributes, 0, 0, 0, pg) == -1) {
- goto error;
+ goto end;
}
if (pg != NULL) {
aldap_freepage(pg);
@@ -443,7 +443,7 @@ ldap_query(const char *filter, const char *key, char **attributes, struct query_
while ((m = aldap_parse(aldap)) != NULL) {
if (aldap->msgid != m->msgid)
- goto error;
+ goto end;
if (m->message_type == LDAP_RES_SEARCH_RESULT) {
if (m->page != NULL && m->page->cookie_len)
pg = m->page;
@@ -453,39 +453,35 @@ ldap_query(const char *filter, const char *key, char **attributes, struct query_
break;
}
if (m->message_type != LDAP_RES_SEARCH_ENTRY)
- goto error;
+ goto end;
if (found >= numresults) {
if (!realloc_results(&res, &numresults)) {
- goto error;
+ goto end;
}
}
memset(&res[found], 0, sizeof(res[found]));
for (i = 0; i < n; ++i)
if (aldap_match_attr(m, attributes[i], &res[found].v[i]) != 1)
- goto error;
+ goto end;
aldap_freemsg(m);
m = NULL;
found++;
}
} while (pg != NULL);
- if (ret == -1)
- goto error;
- ret = found;
- *results = res;
- goto end;
-
-error:
- for (i = 0; i < found; i++) {
- for (j = 0; j < n; j++) {
- aldap_free_attr(res[i].v[j]);
+end:
+ if (ret == -1) {
+ for (i = 0; i < found; i++) {
+ for (j = 0; j < n; j++) {
+ aldap_free_attr(res[i].v[j]);
+ }
}
+ free(res);
+ } else {
+ ret = found;
+ *results = res;
}
- free(res);
- ret = -1;
-
-end:
if (m)
aldap_freemsg(m);
log_debug("debug: table_ldap: ldap_query: filter=%s, ret=%d", filter, ret);
--
2.39.2
