Chris Monson wrote:
(patch)
A couple of comments:
For the representation of the config directive
+ #define IP_LOOKUP_DEFAULT 0 + #define IP_LOOKUP_ALL 1 + #define IP_LOOKUP_IPV4OKAY 2 + #define IP_LOOKUP_IPV6OKAY 3 + unsigned int ip_lookups : 4; +
Why not just store whatever flags should be passed in to apr_sockaddr_info_get() (or rather, whatever flags should be OR-ed with anything else the module needs to specify)? As I think you mentioned before, new bit values would be needed if we added IPv6Only or IPv4Only in the future. (Not a big deal, but I just don't see the benefit to using bit field here. Any other opinions from the peanut gallery?)
I thought about doing that, but eventually decided against it because the configuration directive may need to store not only flag information, but family information. It seemed to make sense to have it mirror the format of the HostNameLookups directive, as well (using a bitfield), so I stuck with something tried and true.
Mostly it's because of the possibility (however unlikely) of adding IPv?Only fields, which would involve either a new structure member or just using more of those 4 allocated bits. It was done in the interest of growth, though it may be overkill. I would be happy to change it to store the actual flags if that is what is wanted.
---/---
In the code below, we already have an IPv4 address and we're getting apr_sockaddr_info_get() to build a representation of it. I don't think the IP lookup flags should be specified here.
/* make the connection */
! apr_sockaddr_info_get(&pasv_addr, apr_psprintf(p, "%d.%d.%d.%d", h3, h2, h1, h0), connect_addr->family, pasvport, 0, p);
rv = apr_connect(data_sock, pasv_addr);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
Excellent point. That was clearly an oversight on my part. I'll fix it as soon as I have a moment :)
That brings up a question, however. I wanted to make the same changes to proxy_util.c, but it seemed like the function was being used in a different way, there. I didn't dig in to find out what all was going on, but I eventually decided against changing that file at all, even though it has one call to apr_sockaddr_info_get that is clearly not checking an IP address (and one that is), but a hostname. The difficulty in that file is that the call does not have enough context available to get the right configuration (the config accessor requires a request object).
That, in turn, brings up another point that I would like to discuss. Currently ap_get_iplookup_flags takes a request object as its only parameter. The reason for this is that I made IPLookups a per-directory config directive just like HostNameLookups. That may have been a poor decision, and I would like some feedback on it.
Finally, the initial thread on this topic indicated that the signature of ap_get_iplookup_flags should be something like this:
void ap_get_iplookup_flags( apr_int32_t *flags, request_rec *r )
As opposed to the way I did it, with an apr_int32_t as the return type. Was there a particular reason for this preference of passing a mutable parameter, or was it just a brain dump? In some ways it may make sense to have two mutable parameters passed in like so:
void ap_get_iplookup_flags( apr_int32_t *flags, apr_int32_t *family, request_rec *r )
That would make it easier to include the IPv4Only and IPv6Only flags in the future (or right now, if that's what we want to do). It's something else that may merit some discussion.
Thanks for your patience as I get my feet wet. The submission process has been an educational experience for me, and I appreciate all of the feedback.
C
