You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Roy T. Fielding" <fi...@gbiv.com> on 2007/05/09 14:18:20 UTC
Re: svn commit: r536291 - in /httpd/httpd/branches/2.2.x: STATUS modules/proxy/proxy_util.c
On May 8, 2007, at 11:25 AM, jim@apache.org wrote:
> +#define USE_ALTERNATE_IS_CONNECTED 1
> +
> +#if !defined(APR_MSG_PEEK) && defined(MSG_PEEK)
> +#define APR_MSG_PEEK MSG_PEEK
> +#endif
> +
> +#if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
Huh? Why are we polluting macro space with useless defines?
....Roy
Re: svn commit: r536291 - in /httpd/httpd/branches/2.2.x: STATUS modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On May 9, 2007, at 8:55 AM, Roy T. Fielding wrote:
> On May 9, 2007, at 5:32 AM, Jim Jagielski wrote:
>> On May 9, 2007, at 8:18 AM, Roy T. Fielding wrote:
>>> On May 8, 2007, at 11:25 AM, jim@apache.org wrote:
>>>> +#define USE_ALTERNATE_IS_CONNECTED 1
>>>> +
>>>> +#if !defined(APR_MSG_PEEK) && defined(MSG_PEEK)
>>>> +#define APR_MSG_PEEK MSG_PEEK
>>>> +#endif
>>>> +
>>>> +#if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
>>>
>>> Huh? Why are we polluting macro space with useless defines?
>>>
>>
>> The USE_ALTERNATE_IS_CONNECTED is to allow that section
>> to be bypassed (although it should be protected
>> by a "ifndef USE_ALTERNATE_IS_CONNECTED" to allow
>> compile time changes I'm guessing).
>
> Well, that would at least give it some reason to exist. If we know
> that new function works, then remove the defines and the old function.
> If it only works when APR_MSG_PEEK is known by APR, then that is
> the only #if needed (and we should not need to define our own).
>
> If we don't know if it works, then -1 on the backport.
>
It's used in mod_jk (a variant, at least)... I've seen no
issues on my testing... I agree that making it conditional
isn't 100% correct, but it's how it's done in trunk
so far and the backport proposed the exact diffs.
>> The APR_MSG_PEEK
>> is so when APR is updated to reflect the
>> existence of APR_MSG_PEEK, we don't need to touch
>> this code. Finally, the older version assumed
>> that MSG_PEEK was defined and the compile would
>> barf if not; this avoid that.
>
> Well, the code won't work anyway if APR doesn't know about MSG_PEEK.
>
> + status = apr_socket_recvfrom(&unused, socket, APR_MSG_PEEK,
> + &buf[0], &len);
>
> presumably means something to APR, since otherwise we will lose data.
>
The previous version passed just MSG_PEEK, which worked, but
was cumbersome. Again, the idea is that apr_socket_recvfrom()
really should know about some basic recvfrom() flags, but
right now it doesn't; well, "know about" the same way
that apr_poll() knows about POLLIN/APR_POLLIN for example.
So right now, since apr_socket_recvfrom() does not, we
simply set APR_MSG_PEEK to MSG_PEEK and be done with
it. I'm fine with saying "until apr_socket_recvfrom()
is updated to support MSG_PEEK we leave this out".
I'm even more fine in saying "until then we leave this
in but don't make it the default (or use some configure
magic to determine the default)". But it seemed
like a cool feature in trunk, and a good candidate
for backport....
>
Re: svn commit: r536291 - in /httpd/httpd/branches/2.2.x: STATUS modules/proxy/proxy_util.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On May 9, 2007, at 5:32 AM, Jim Jagielski wrote:
> On May 9, 2007, at 8:18 AM, Roy T. Fielding wrote:
>> On May 8, 2007, at 11:25 AM, jim@apache.org wrote:
>>> +#define USE_ALTERNATE_IS_CONNECTED 1
>>> +
>>> +#if !defined(APR_MSG_PEEK) && defined(MSG_PEEK)
>>> +#define APR_MSG_PEEK MSG_PEEK
>>> +#endif
>>> +
>>> +#if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
>>
>> Huh? Why are we polluting macro space with useless defines?
>>
>
> The USE_ALTERNATE_IS_CONNECTED is to allow that section
> to be bypassed (although it should be protected
> by a "ifndef USE_ALTERNATE_IS_CONNECTED" to allow
> compile time changes I'm guessing).
Well, that would at least give it some reason to exist. If we know
that new function works, then remove the defines and the old function.
If it only works when APR_MSG_PEEK is known by APR, then that is
the only #if needed (and we should not need to define our own).
If we don't know if it works, then -1 on the backport.
> The APR_MSG_PEEK
> is so when APR is updated to reflect the
> existence of APR_MSG_PEEK, we don't need to touch
> this code. Finally, the older version assumed
> that MSG_PEEK was defined and the compile would
> barf if not; this avoid that.
Well, the code won't work anyway if APR doesn't know about MSG_PEEK.
+ status = apr_socket_recvfrom(&unused, socket, APR_MSG_PEEK,
+ &buf[0], &len);
presumably means something to APR, since otherwise we will lose data.
....Roy
Re: svn commit: r536291 - in /httpd/httpd/branches/2.2.x: STATUS modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On May 9, 2007, at 8:18 AM, Roy T. Fielding wrote:
> On May 8, 2007, at 11:25 AM, jim@apache.org wrote:
>> +#define USE_ALTERNATE_IS_CONNECTED 1
>> +
>> +#if !defined(APR_MSG_PEEK) && defined(MSG_PEEK)
>> +#define APR_MSG_PEEK MSG_PEEK
>> +#endif
>> +
>> +#if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
>
> Huh? Why are we polluting macro space with useless defines?
>
The USE_ALTERNATE_IS_CONNECTED is to allow that section
to be bypassed (although it should be protected
by a "ifndef USE_ALTERNATE_IS_CONNECTED" to allow
compile time changes I'm guessing). The APR_MSG_PEEK
is so when APR is updated to reflect the
existence of APR_MSG_PEEK, we don't need to touch
this code. Finally, the older version assumed
that MSG_PEEK was defined and the compile would
barf if not; this avoid that.