On 2024/01/23 19:49:34 +0100, Philipp <phil...@bureaucracy.de> wrote:
> [2024-01-23 11:39] Omar Polo <o...@omarpolo.com>
> > spotted while reading Philipp' ldaps diff. it's really ugly to reach
> > into the struct sockaddrs when using getaddrinfo()...
>
> Nice this makes the libtls integration simpler. Also some comments inline.
>
> > however, I don't use ldap so this could use at least some testing :)
>
> No problem I can test this.
thanks :)
even if we'll change the transport away from imsg, it could still be
useful to improve these extras I think. (again, don't know much of ldap,
nor can judge the state of tha table-ldap; just scratching an itch after
taking a look at your diff.)
> > @@ -85,8 +85,8 @@ ldap_connect(const char *addr)
> > {
> > struct aldap_url lu;
> > struct addrinfo hints, *res0, *res;
> > - char *buf;
> > - int error, fd = -1;
> > + char *buf, port[32];
>
> nitpick: the port is max 65535, so 8 byte would be enough.
right, i picked the first power of two that came to mind. it could be
interesting however to change the parsing function to keep the port as
string, to avoid this conversion here.
> [...]
> > + if (connect(fd, res->ai_addr, res->ai_addrlen) == 0)
> > + return aldap_init(fd);
>
> Here a aldap_free_url() is missing, therefor lu.buffer is leaking.
> But currently aldap_free_url() is buggy, it frees lu->buffer and
> lu->filter but this is the same object.
Yes, but it's the same behaviour as before. I didn't want to change
many things at a time.
if you're interested in this however, we can also avoid the strdup()
here since aldap_parse_url() already strdup()s the string for parsing
(but still frees the passed argument...)
