Re: New external_acl helper squid_kerb_ldap

On Sun, 30 May 2010 16:15:14 +0100, "Markus Moeller"
<huaraz_at_moeller.plus.com> wrote:
> Hi,
>
> I have converted my helper to kerberos_ldap_group ( not sure if that
is
> the best name) and created a patch for inclusion into the head revision.

> Please review and let me know any feedback.
>
>
> Thank you
> Markus

Hi Markus,

 * No need to include the COPYING file when bundled, Squid gets
distributed with a version in the base directory.
 * Also seems to have pulled in "negotiate_kerberos_auth-logging.patch"
and "negotiate_kerberos_auth-new.cc"

support.h:
 * config.h will pull in the compat/* files. no need to include them
explicitly now. it will also pull in several of the standard headers.
 * not sure about that LogTime definition going in here when its not
inline. Can it be moved to a .cc?

kerberos_ldap_group.cc and support_*.cc:
 * needs to include config.h as the first include, this will pull in the
basic headers for types etc for portability: unistd.h, stdlib.h,
sys/time.h, etc

 * ALL system headers used need to be wrapped with HAVE_* even if standard
headers, and go below the local file includes. If a .h file needs some
definition to build, the system file needs to be pulled into there instead
of the .cc.

Also; why are the support_*.cc files all in helpers/external_acl/
directly?

support_ldap.cc:
 * sub-Copyright for The OpenLDAP Foundation will have to be added to the
Squid combined CREDITS copyright file.

support_netbios.cc:
 * I thought NetBOIS was officially declared dead by MS? can that be
dropped?

support_resolv.cc:
 * RFC1035 text is bundled with Squid in docs/rfc/, IMO a simple reference
to the RFC number and section numbers should be sufficient for the
documentation.
 * RFC 2782 can be pulled into doc/rfc/ and added to that
doc/rfc/1-index.txt as a separate patch instead of quoting large portions.
 * There are rfc1035 type definitions and packers/unpackers are provided
through $(COMPAT_LIB) for now. see include/rfc1035.h if you actually need
them. Though from a quick look you seem to be using the resolver library
API not packet-level details from RFC 1035.
 * NP: long-term plan is to have a libdns in Squid you can link and call,
but that is not ready yet, so overlooking the manual resolving this file
does for now.

 * Some good news; since we imported the auth helper I've cleaned up
Squids' API for getaddrinfo/freeaddrinfo/gai_strerror. You can use the
POSIX names now instead of the xgetaddrinfo variants.

Amos
Received on Mon May 31 2010 - 01:11:55 MDT