You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by mt...@apache.org on 2006/09/12 08:59:35 UTC
svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Author: mturk
Date: Mon Sep 11 23:59:34 2006
New Revision: 442492
URL: http://svn.apache.org/viewvc?view=rev&rev=442492
Log:
Backport r442135 from trunk.
Modified:
apr/apr/branches/1.2.x/network_io/unix/sockets.c
Modified: apr/apr/branches/1.2.x/network_io/unix/sockets.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/network_io/unix/sockets.c?view=diff&rev=442492&r1=442491&r2=442492
==============================================================================
--- apr/apr/branches/1.2.x/network_io/unix/sockets.c (original)
+++ apr/apr/branches/1.2.x/network_io/unix/sockets.c Mon Sep 11 23:59:34 2006
@@ -176,29 +176,34 @@
apr_status_t apr_socket_accept(apr_socket_t **new, apr_socket_t *sock,
apr_pool_t *connection_context)
{
- alloc_socket(new, connection_context);
- set_socket_vars(*new, sock->local_addr->sa.sin.sin_family, SOCK_STREAM, sock->protocol);
+ int s;
+ apr_sockaddr_t sa;
-#ifndef HAVE_POLL
- (*new)->connected = 1;
-#endif
- (*new)->timeout = -1;
-
- (*new)->socketdes = accept(sock->socketdes,
- (struct sockaddr *)&(*new)->remote_addr->sa,
- &(*new)->remote_addr->salen);
+ apr_sockaddr_vars_set(&sa, sock->local_addr->sa.sin.sin_family, 0);
+ sa.pool = connection_context;
+ s = accept(sock->socketdes, (struct sockaddr *)&sa.sa, &sa.salen);
- if ((*new)->socketdes < 0) {
+ if (s < 0) {
return errno;
}
#ifdef TPF
- if ((*new)->socketdes == 0) {
+ if (s == 0) {
/* 0 is an invalid socket for TPF */
return APR_EINTR;
}
#endif
+ alloc_socket(new, connection_context);
+ set_socket_vars(*new, sock->local_addr->sa.sin.sin_family, SOCK_STREAM, sock->protocol);
+
+#ifndef HAVE_POLL
+ (*new)->connected = 1;
+#endif
+ (*new)->timeout = -1;
(*new)->remote_addr_unknown = 0;
+
+ (*new)->socketdes = s;
+ *(*new)->remote_addr = sa;
*(*new)->local_addr = *sock->local_addr;
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
mturk@apache.org wrote:
> Author: mturk
> Date: Mon Sep 11 23:59:34 2006
> New Revision: 442492
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=442492
> Log:
> Backport r442135 from trunk.
If you are backporting bug fixes, would you mind keeping 0.9.x in sync,
since it's very widely distributed today?
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
mturk@apache.org wrote:
> Author: mturk
> Date: Mon Sep 11 23:59:34 2006
> New Revision: 442492
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=442492
> Log:
> Backport r442135 from trunk.
If you are backporting bug fixes, would you mind keeping 0.9.x in sync,
since it's very widely distributed today?
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Mladen Turk <mt...@apache.org>.
William A. Rowe, Jr. wrote:
>
> Ok - reading Mladen's and your comments, I'll agree the extra copy is
> warrented, let's proceed on your plan...
>
> * revert the original patch from 0.9, 1.2 branches for the moment
> * let this patch settle a bit
> * backport correct/complete patch
>
> Or did I misunderstand you?
>
The current trunk works just fine.
I was able to reproduce the problems with my original
patch that Joe reported (the reason was because the
original patch copied the entire apr_sockaddr_t together
with pointers set to the stack temp variable).
I suggest we simply backport 447894 to 1.2 and 0.9
because it was derived from the original 442135 and
443262 already backported.
Regards,
Mladen.
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> Gah, just realised I never dealt with this, sorry :(
>
> Since this is really an attempt to make a new API guarantee, it is
> something that can only be done in a minor version bump, and would need
> to be done for the other implementations too.
Well, I'm a little torn. I don't mind offering the 'feature' of not wasting
the memory on failure (now that I know it isn't consistent across platforms
already). You are half right though - They can't test for 1.2 for example
and expect the API to behave one way or the other.
In 1.3 of course, we can add 'promises' on that behavior.
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Sep 20, 2006 at 02:04:18PM -0500, William Rowe wrote:
> Joe Orton wrote:
> >
> > If the API guarantee is "may allocate memory on failure" then the fix is
> > bad and should be reverted; the caller will have to do the
> > create/destroy dance anyway so it's needless overhead whether large or
> > small.
> >
> > If the API guarantee is "will not allocate memory on failure" then
> > clearly this fix is necessary. I think this option makes more sense.
>
> Ok - reading Mladen's and your comments, I'll agree the extra copy is
> warrented, let's proceed on your plan...
>
> * revert the original patch from 0.9, 1.2 branches for the moment
> * let this patch settle a bit
> * backport correct/complete patch
>
> Or did I misunderstand you?
Gah, just realised I never dealt with this, sorry :(
Since this is really an attempt to make a new API guarantee, it is
something that can only be done in a minor version bump, and would need
to be done for the other implementations too.
joe
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> If the API guarantee is "may allocate memory on failure" then the fix is
> bad and should be reverted; the caller will have to do the
> create/destroy dance anyway so it's needless overhead whether large or
> small.
>
> If the API guarantee is "will not allocate memory on failure" then
> clearly this fix is necessary. I think this option makes more sense.
Ok - reading Mladen's and your comments, I'll agree the extra copy is
warrented, let's proceed on your plan...
* revert the original patch from 0.9, 1.2 branches for the moment
* let this patch settle a bit
* backport correct/complete patch
Or did I misunderstand you?
Bill
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
>
> If the API guarantee is "may allocate memory on failure" then the fix is
> bad and should be reverted; the caller will have to do the
> create/destroy dance anyway so it's needless overhead whether large or
> small.
>
This was the case without my patch to unix/sockets.c
I was "will not allocate on Win32, will always allocate on unix".
> If the API guarantee is "will not allocate memory on failure" then
> clearly this fix is necessary. I think this option makes more sense.
>
Of course we can have "will always allocate memory regardless of failure"
which was the case before the pacth (unix only), but like said it's resource
wasteful, and IMO we all agree on that.
Regards,
Mladen.
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Sep 20, 2006 at 12:48:56PM +0200, Mladen Turk wrote:
> William A. Rowe, Jr. wrote:
> >Joe Orton wrote:
> >
> >Yes - please do. One aspect of this patch I strongly dislike is the fact
> >that it's much less efficient. If the pool associated with the soon-to-be
> >allocated apr_socket_t would be destroyed on accept failure (the TYPICAL
> >design paradigm) there's no reason to avoid this allocation, and we've
> >watched sa buffers grow as the network layer has become richer (e.g. IPV6)
> >which makes stack a suboptimal place to place such buffers.
> >
"much less efficient" seems a bit strong - the performance difference is
a larger stack frame and an additional ~128 byte memcpy which I expect
is totally lost in the noise for this function, or have you benchmarked
it?
> So, either we'll have the Win32 way of doing accept or the unix one.
> I personally think that not allocating apr_socket_t if the accept
> fails would give the simpler programming interface without
> worrying on memory usage, and that would at least allow much
> faster non-blocking socket accept without the need to create/destroy
> pools on each APR_EAGAIN.
As usual what is lacking is any kind of strong API guarantee, just "hey
this function uses a pool. good luck out there".
If the API guarantee is "may allocate memory on failure" then the fix is
bad and should be reverted; the caller will have to do the
create/destroy dance anyway so it's needless overhead whether large or
small.
If the API guarantee is "will not allocate memory on failure" then
clearly this fix is necessary. I think this option makes more sense.
joe
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Mladen Turk <mt...@apache.org>.
William A. Rowe, Jr. wrote:
> Joe Orton wrote:
>
> Yes - please do. One aspect of this patch I strongly dislike is the fact
> that it's much less efficient. If the pool associated with the soon-to-be
> allocated apr_socket_t would be destroyed on accept failure (the TYPICAL
> design paradigm) there's no reason to avoid this allocation, and we've
> watched sa buffers grow as the network layer has become richer (e.g. IPV6)
> which makes stack a suboptimal place to place such buffers.
>
I agree, but then all platforms must behave in the same fashion.
Without the patch, the Win32 does not allocate the socket in case
accept fails, while unix and others does.
So, either we'll have the Win32 way of doing accept or the unix one.
I personally think that not allocating apr_socket_t if the accept
fails would give the simpler programming interface without
worrying on memory usage, and that would at least allow much
faster non-blocking socket accept without the need to create/destroy
pools on each APR_EAGAIN.
However, any mode is fine if its consistent across the platforms,
which is not the case right now.
Regards,
Mladen.
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Wed, Sep 13, 2006 at 01:30:06PM +0100, Joe Orton wrote:
>> On Tue, Sep 12, 2006 at 06:59:35AM -0000, Mladen Turk wrote:
>>> Author: mturk
>>> Date: Mon Sep 11 23:59:34 2006
>>> New Revision: 442492
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=442492
>>> Log:
>>> Backport r442135 from trunk.
>> This has broken the access control tests in httpd-test for both 2.2.x
>> and 2.0.x. I'm seeing random addresses logged in access_log, 0.0.0.0
>> and such. The ->remote_addr changes below look unrelated to the
>> original change, and I guess that is what is wrong.
>
> I've fixed this but I still have far less confidence in these changes
> than would warrant putting them in 0.9.x/1.2.x. IMO revert the branch
> changes and just leave this on the trunk to soak for a bit.
Yes - please do. One aspect of this patch I strongly dislike is the fact
that it's much less efficient. If the pool associated with the soon-to-be
allocated apr_socket_t would be destroyed on accept failure (the TYPICAL
design paradigm) there's no reason to avoid this allocation, and we've
watched sa buffers grow as the network layer has become richer (e.g. IPV6)
which makes stack a suboptimal place to place such buffers.
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Sep 13, 2006 at 01:30:06PM +0100, Joe Orton wrote:
> On Tue, Sep 12, 2006 at 06:59:35AM -0000, Mladen Turk wrote:
> > Author: mturk
> > Date: Mon Sep 11 23:59:34 2006
> > New Revision: 442492
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=442492
> > Log:
> > Backport r442135 from trunk.
>
> This has broken the access control tests in httpd-test for both 2.2.x
> and 2.0.x. I'm seeing random addresses logged in access_log, 0.0.0.0
> and such. The ->remote_addr changes below look unrelated to the
> original change, and I guess that is what is wrong.
I've fixed this but I still have far less confidence in these changes
than would warrant putting them in 0.9.x/1.2.x. IMO revert the branch
changes and just leave this on the trunk to soak for a bit.
joe
Re: svn commit: r442492 - /apr/apr/branches/1.2.x/network_io/unix/sockets.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Sep 12, 2006 at 06:59:35AM -0000, Mladen Turk wrote:
> Author: mturk
> Date: Mon Sep 11 23:59:34 2006
> New Revision: 442492
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=442492
> Log:
> Backport r442135 from trunk.
This has broken the access control tests in httpd-test for both 2.2.x
and 2.0.x. I'm seeing random addresses logged in access_log, 0.0.0.0
and such. The ->remote_addr changes below look unrelated to the
original change, and I guess that is what is wrong.
> Modified:
> apr/apr/branches/1.2.x/network_io/unix/sockets.c
>
> Modified: apr/apr/branches/1.2.x/network_io/unix/sockets.c
> URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/network_io/unix/sockets.c?view=diff&rev=442492&r1=442491&r2=442492
> ==============================================================================
> --- apr/apr/branches/1.2.x/network_io/unix/sockets.c (original)
> +++ apr/apr/branches/1.2.x/network_io/unix/sockets.c Mon Sep 11 23:59:34 2006
> @@ -176,29 +176,34 @@
> apr_status_t apr_socket_accept(apr_socket_t **new, apr_socket_t *sock,
> apr_pool_t *connection_context)
> {
> - alloc_socket(new, connection_context);
> - set_socket_vars(*new, sock->local_addr->sa.sin.sin_family, SOCK_STREAM, sock->protocol);
> + int s;
> + apr_sockaddr_t sa;
>
> -#ifndef HAVE_POLL
> - (*new)->connected = 1;
> -#endif
> - (*new)->timeout = -1;
> -
> - (*new)->socketdes = accept(sock->socketdes,
> - (struct sockaddr *)&(*new)->remote_addr->sa,
> - &(*new)->remote_addr->salen);
> + apr_sockaddr_vars_set(&sa, sock->local_addr->sa.sin.sin_family, 0);
> + sa.pool = connection_context;
> + s = accept(sock->socketdes, (struct sockaddr *)&sa.sa, &sa.salen);
>
> - if ((*new)->socketdes < 0) {
> + if (s < 0) {
> return errno;
> }
> #ifdef TPF
> - if ((*new)->socketdes == 0) {
> + if (s == 0) {
> /* 0 is an invalid socket for TPF */
> return APR_EINTR;
> }
> #endif
> + alloc_socket(new, connection_context);
> + set_socket_vars(*new, sock->local_addr->sa.sin.sin_family, SOCK_STREAM, sock->protocol);
> +
> +#ifndef HAVE_POLL
> + (*new)->connected = 1;
> +#endif
> + (*new)->timeout = -1;
>
> (*new)->remote_addr_unknown = 0;
> +
> + (*new)->socketdes = s;
> + *(*new)->remote_addr = sa;
>
> *(*new)->local_addr = *sock->local_addr;
>
>