Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of trawick@gmail.com designates
 209.85.212.45 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAKUrXK45qynd5bu=nShdWZSGQCzMP5A4HNKEtqDjgKKpG2wTPQ@mail.gmail.com>
References: 
 <31f247cbaff240b7a06a2c3713fb8a96@BY2PR03MB608.namprd03.prod.outlook.com>
	<f6023cb29d354d6fa3aacfa34e759d4c@BY2PR03MB608.namprd03.prod.outlook.com>
	<CAKUrXK45qynd5bu=nShdWZSGQCzMP5A4HNKEtqDjgKKpG2wTPQ@mail.gmail.com>
Date: Thu, 9 Aug 2012 16:28:49 -0400
Message-ID: 
 <CAKUrXK7kg4m0x8UTNPgT=xzH9DSfKUA0X0v0fHx65FRePuKHgQ@mail.gmail.com>
Subject: Re: Fix for Windows bug#52476
From: Jeff Trawick <trawick@gmail.com>
To: dev@httpd.apache.org
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

On Thu, Aug 9, 2012 at 3:52 PM, Jeff Trawick <trawick@gmail.com> wrote:
> On Thu, Aug 9, 2012 at 2:26 PM, Claudio Caldato (MS OPEN TECH)
> <claudioc@microsoft.com> wrote:
>> Better patch, fixed minor issue after another code review.
>
> I tested and seemed to get good results, but my testing isn't
> reproducible enough with/without various patches to be conclusive.
>
> A couple of comments on the patch itself:
>
> * Why is BytesRead initialized to 0 in the other function?  (I didn't
> include that part of your patch in my test.)
> * Pass apr_get_netos_error() to ap_log_error instead of rc after a
> Winsock function like recv() fails.  (apr_get_os_error() otherwise)
>
> More comments below:
>
>
>> Thanks
>>
>> Claudio
>
>> From: Claudio Caldato (MS OPEN TECH) [mailto:claudioc@microsoft.com]
>> Sent: Thursday, August 9, 2012 11:13 AM
>> To: dev@httpd.apache.org
>> Subject: Fix for Windows bug#52476
>>
>>
>>
>> Please code review the fix and let me know if you find any issue.
>>
>>
>>
>> Attached is the proposed patch for
>>
>> server\mpm\winnt\child.c
>>
>>
>>
>> Summary for code reviewers:
>>
>> If AcceptFilter is =91connect=92 or =91none=92, we read data from socket=
 on worker
>> thread. We use blocking recv and assign context->overlapped.Pointer to h=
eap
>> allocated buffer. It is the same procedure as in case of =91AcceptFilter
>> data=92, but done in worker thread to keep listen thread unblocked.
>
> The big picture doesn't seem correct.  The main path of httpd (that
> stuff called via ap_run_process_connection()) needs to be able to read
> at will.  It shouldn't help to move a read() to before the call to
> ap_run_process_connection().
>
>
>> Note:
>>
>> It looks like context with overlapped.Pointer =3D=3D NULL is not process=
ed
>> correctly in windows version of httpd. It may be related to the fact tha=
t
>> winnt_insert_network_bucket() rejects context records with
>> overlapped.Pointer =3D=3D NULL
>
> Uhh, maybe I'm confused but that sounds like the issue!  (I'm not
> familiar with that hook.)
>
> What about this patch?
>
> Index: server/mpm/winnt/child.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- server/mpm/winnt/child.c    (revision 1371377)
> +++ server/mpm/winnt/child.c    (working copy)
> @@ -780,11 +780,15 @@
>      apr_bucket *e;
>      winnt_conn_ctx_t *context =3D ap_get_module_config(c->conn_config,
>                                                       &mpm_winnt_module);
> -    if (context =3D=3D NULL || (e =3D context->overlapped.Pointer) =3D=
=3D NULL)
> +    if (context =3D=3D NULL) {
>          return AP_DECLINED;
> +    }
>
> -    /* seed the brigade with AcceptEx read heap bucket */
> -    APR_BRIGADE_INSERT_HEAD(bb, e);
> +    if ((e =3D context->overlapped.Pointer) !=3D NULL) {
> +        /* seed the brigade with AcceptEx read heap bucket */
> +        APR_BRIGADE_INSERT_HEAD(bb, e);
> +    }
> +
>      /* also seed the brigade with the client socket. */
>      e =3D apr_bucket_socket_create(socket, c->bucket_alloc);
>      APR_BRIGADE_INSERT_TAIL(bb, e);

no, that's wrong

you only need to put the socket bucket there if you need to put data
in front of it; (when declining, core's insert_network_bucket hook
will do the right thing with the socket)

AFAICT there's no functional issue with winnt_insert_network_bucket,
though it could defer to core to handle the socket bucket

>
>>
>>
>>
>>
>>
>> Please advise on what the next step(s) should be.
>>
>>
>>
>> Thanks
>>
>> Claudio
>>
>>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/


--=20
Born in Roswell... married an alien...
http://emptyhammock.com/