On Sun, 2026-03-22 at 18:38 -0500, Andrew Jackson wrote: > Noticed 1 variable that was unused during non-LDAP builds. Tested > locally and did not see the error/warning. Also some minor cleanup > (comments, definition placement, etc).
I don't have an LDAP server handy, so I couldn't test the patch, but I read through it. I think that this is a useful addition. +#ifdef USE_LDAP + if (ldapservice != NULL) + if (strncmp(ldapservice, "ldap", 4) == 0) + if (!ldapServiceLookup(ldapservice, options, errorMessage)) + return 0; +#endif I think that the test if the string starts with "ldap" is midguided. It was probably copied from the other call site of ldapServiceLookup(), but there it is necessary, because an entry in the connection service file could start with something else. A value for the parameter "ldapservice" that doesn't start with "ldap" should cause an error. ldapServiceLookup() checks if the string starts with "ldap://"; and throws an error if it doesn't. So I'd say that you should simply remove the test if the string starts with "ldap". I also think that it is wrong to return 0 (success) if ldapServiceLookup() fails. Why should it be OK to specify a bad LDAP URL? I don't understand why that code is in parseServiceInfo(). After all, it has nothing to do with a connection service file. Calling it from conninfo_add_defaults() would make more sense to me. For the same reason, I am not entirely happy with the name "ldapservice", but I can't think of anything better. The way your code is now, "ldapservice" sets default values that get overridden by explicitly named parameters. I think the documentation should mention that. Yours, Laurenz Albe
