On Sun, 30 May 2010 16:15:14 +0100, "Markus Moeller" <[email protected]> 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
