httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Monson <ch...@orangatango.net>
Subject Re: mod_proxy and AAAA queries
Date Mon, 03 Mar 2003 15:11:56 GMT
> 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

>


Mime
View raw message