On 2024/01/27 11:49:52 +0100, Philipp <phil...@bureaucracy.de> wrote:
> Hi
>
> I have some smal patches for the table-ldap. These are based on the
> getaddrinfo patch from Olmar (and the two fixups from me).
The seemed fine to me, all except the last one.
Your fix is indeed correct, but I'd prefer to adjust also some lines
around your changes as well:
> diff /home/op/w/opensmtpd-extras
> commit - cf39e3c8f04a9e95e4765cf4d335ca1878bb99f3
> path + /home/op/w/opensmtpd-extras
> blob - 5a11e723f1ddba97c94644ac8b50ed5096b0f932
> file + extras/tables/table-ldap/table_ldap.c
> --- extras/tables/table-ldap/table_ldap.c
> +++ extras/tables/table-ldap/table_ldap.c
> @@ -334,15 +334,15 @@ table_ldap_lookup(int service, struct dict *params, co
> case K_NETADDR:
> if ((ret = ldap_run_query(service, key, dst, sz)) > 0) {
Here it should be >= 0, not > 0.
quick recap: the table_* functions return -1 on failure, 0 when there
were no results and 1 when there is a result.
Here we attemp to reconnect when there simply wasn't a result for the
query, which I believe it's wrong.
> return ret;
> }
> log_debug("debug: table-ldap: reconnecting");
> if (!(ret = ldap_open())) {
Also, here it's useless to save the return value, since ldap_open()
returns either zero or one.
> log_warnx("warn: table-ldap: failed to connect");
> - return ret;
> + return -1;
> }
> return ldap_run_query(service, key, dst, sz);
> default:
> return -1;
> }
> }
>
The same applies to the _check function as well.
Again, it's not a problem introduced by your diff, but since you're
fixing stuff in here would you mind to fix also these points?
Thanks,
Omar Polo
